* [PATCH] led: try to get LED's label from the 'label' property @ 2014-07-27 8:35 Antony Pavlov 2014-07-28 5:49 ` Sascha Hauer 0 siblings, 1 reply; 7+ messages in thread From: Antony Pavlov @ 2014-07-27 8:35 UTC (permalink / raw) To: barebox Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> --- drivers/led/led-gpio.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/led/led-gpio.c b/drivers/led/led-gpio.c index 7bb3b49..cddac08 100644 --- a/drivers/led/led-gpio.c +++ b/drivers/led/led-gpio.c @@ -212,7 +212,9 @@ static int led_gpio_of_probe(struct device_d *dev) continue; gled = xzalloc(sizeof(*gled)); - gled->led.name = xstrdup(child->name); + gled->led.name = xstrdup(of_get_property(child, "label", NULL)); + if (!gled->led.name) + gled->led.name = xstrdup(child->name); gled->gpio = gpio; gled->active_low = (flags & OF_GPIO_ACTIVE_LOW) ? 1 : 0; -- 2.0.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] led: try to get LED's label from the 'label' property 2014-07-27 8:35 [PATCH] led: try to get LED's label from the 'label' property Antony Pavlov @ 2014-07-28 5:49 ` Sascha Hauer 2014-07-28 6:22 ` Antony Pavlov 0 siblings, 1 reply; 7+ messages in thread From: Sascha Hauer @ 2014-07-28 5:49 UTC (permalink / raw) To: Antony Pavlov; +Cc: barebox On Sun, Jul 27, 2014 at 12:35:52PM +0400, Antony Pavlov wrote: > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> > --- > drivers/led/led-gpio.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/led/led-gpio.c b/drivers/led/led-gpio.c > index 7bb3b49..cddac08 100644 > --- a/drivers/led/led-gpio.c > +++ b/drivers/led/led-gpio.c > @@ -212,7 +212,9 @@ static int led_gpio_of_probe(struct device_d *dev) > continue; > > gled = xzalloc(sizeof(*gled)); > - gled->led.name = xstrdup(child->name); > + gled->led.name = xstrdup(of_get_property(child, "label", NULL)); When the property doesn't exist you pass NULL to xstrdup. This will crash. You'll have to add a check or make xstrdup tolerate NULL pointers. Sascha > + if (!gled->led.name) > + gled->led.name = xstrdup(child->name); > gled->gpio = gpio; > gled->active_low = (flags & OF_GPIO_ACTIVE_LOW) ? 1 : 0; > > -- > 2.0.1 > > > _______________________________________________ > barebox mailing list > barebox@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/barebox > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] led: try to get LED's label from the 'label' property 2014-07-28 5:49 ` Sascha Hauer @ 2014-07-28 6:22 ` Antony Pavlov 2014-07-28 6:29 ` Sascha Hauer 2014-07-28 6:34 ` Sebastian Hesselbarth 0 siblings, 2 replies; 7+ messages in thread From: Antony Pavlov @ 2014-07-28 6:22 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox On Mon, 28 Jul 2014 07:49:17 +0200 Sascha Hauer <s.hauer@pengutronix.de> wrote: > On Sun, Jul 27, 2014 at 12:35:52PM +0400, Antony Pavlov wrote: > > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> > > --- > > drivers/led/led-gpio.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/led/led-gpio.c b/drivers/led/led-gpio.c > > index 7bb3b49..cddac08 100644 > > --- a/drivers/led/led-gpio.c > > +++ b/drivers/led/led-gpio.c > > @@ -212,7 +212,9 @@ static int led_gpio_of_probe(struct device_d *dev) > > continue; > > > > gled = xzalloc(sizeof(*gled)); > > - gled->led.name = xstrdup(child->name); > > + gled->led.name = xstrdup(of_get_property(child, "label", NULL)); > > When the property doesn't exist you pass NULL to xstrdup. This will > crash. My bad. I have seen barebox strdup() realization but missed xstrdup() realization. Barebox' strdup() can handle NULL argument: char * strdup(const char *s) { char *new; if ((s == NULL) || ((new = malloc (strlen(s) + 1)) == NULL) ) { return NULL; } strcpy (new, s); return new; } But xstrdup() can't do so: char *xstrdup(const char *s) { char *p = strdup(s); if (!p) panic("ERROR: out of memory\n"); return p; } Can I just change first xstrdup() to strdup() in my patch? E.g. - gled->led.name = xstrdup(of_get_property(child, "label", NULL)); + gled->led.name = strdup(of_get_property(child, "label", NULL)); if (!gled->led.name) gled->led.name = xstrdup(child->name); > You'll have to add a check or make xstrdup tolerate NULL > pointers. > > Sascha > > > + if (!gled->led.name) > > + gled->led.name = xstrdup(child->name); > > gled->gpio = gpio; > > gled->active_low = (flags & OF_GPIO_ACTIVE_LOW) ? 1 : 0; > > > > -- > > 2.0.1 > > > > > > _______________________________________________ > > barebox mailing list > > barebox@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/barebox > > > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- -- Best regards, Antony Pavlov _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] led: try to get LED's label from the 'label' property 2014-07-28 6:22 ` Antony Pavlov @ 2014-07-28 6:29 ` Sascha Hauer 2014-07-28 7:02 ` Antony Pavlov 2014-07-28 6:34 ` Sebastian Hesselbarth 1 sibling, 1 reply; 7+ messages in thread From: Sascha Hauer @ 2014-07-28 6:29 UTC (permalink / raw) To: Antony Pavlov; +Cc: barebox On Mon, Jul 28, 2014 at 10:22:43AM +0400, Antony Pavlov wrote: > On Mon, 28 Jul 2014 07:49:17 +0200 > Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > On Sun, Jul 27, 2014 at 12:35:52PM +0400, Antony Pavlov wrote: > > > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> > > > --- > > > drivers/led/led-gpio.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/led/led-gpio.c b/drivers/led/led-gpio.c > > > index 7bb3b49..cddac08 100644 > > > --- a/drivers/led/led-gpio.c > > > +++ b/drivers/led/led-gpio.c > > > @@ -212,7 +212,9 @@ static int led_gpio_of_probe(struct device_d *dev) > > > continue; > > > > > > gled = xzalloc(sizeof(*gled)); > > > - gled->led.name = xstrdup(child->name); > > > + gled->led.name = xstrdup(of_get_property(child, "label", NULL)); > > > > When the property doesn't exist you pass NULL to xstrdup. This will > > crash. > > My bad. I have seen barebox strdup() realization but missed xstrdup() realization. > > Barebox' strdup() can handle NULL argument: > > char * strdup(const char *s) > { > char *new; > > if ((s == NULL) || > ((new = malloc (strlen(s) + 1)) == NULL) ) { > return NULL; > } > > strcpy (new, s); > return new; > } > > But xstrdup() can't do so: > > char *xstrdup(const char *s) > { > char *p = strdup(s); > > if (!p) > panic("ERROR: out of memory\n"); > return p; > } > > Can I just change first xstrdup() to strdup() in my patch? Why not add a NULL pointer check to xstrdup? > > E.g. > > - gled->led.name = xstrdup(of_get_property(child, "label", NULL)); > + gled->led.name = strdup(of_get_property(child, "label", NULL)); > if (!gled->led.name) > gled->led.name = xstrdup(child->name); I think this code will raise questions. When reading this code one could easily ask why stdrup is used in the first place and xstrdup in the second. A year ahead even we may not know the answer without thinking about it ;) Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] led: try to get LED's label from the 'label' property 2014-07-28 6:29 ` Sascha Hauer @ 2014-07-28 7:02 ` Antony Pavlov 0 siblings, 0 replies; 7+ messages in thread From: Antony Pavlov @ 2014-07-28 7:02 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox On Mon, 28 Jul 2014 08:29:13 +0200 Sascha Hauer <s.hauer@pengutronix.de> wrote: > On Mon, Jul 28, 2014 at 10:22:43AM +0400, Antony Pavlov wrote: > > On Mon, 28 Jul 2014 07:49:17 +0200 > > Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > > > On Sun, Jul 27, 2014 at 12:35:52PM +0400, Antony Pavlov wrote: > > > > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> > > > > --- > > > > drivers/led/led-gpio.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/led/led-gpio.c b/drivers/led/led-gpio.c > > > > index 7bb3b49..cddac08 100644 > > > > --- a/drivers/led/led-gpio.c > > > > +++ b/drivers/led/led-gpio.c > > > > @@ -212,7 +212,9 @@ static int led_gpio_of_probe(struct device_d *dev) > > > > continue; > > > > > > > > gled = xzalloc(sizeof(*gled)); > > > > - gled->led.name = xstrdup(child->name); > > > > + gled->led.name = xstrdup(of_get_property(child, "label", NULL)); > > > > > > When the property doesn't exist you pass NULL to xstrdup. This will > > > crash. > > > > My bad. I have seen barebox strdup() realization but missed xstrdup() realization. > > > > Barebox' strdup() can handle NULL argument: > > > > char * strdup(const char *s) > > { > > char *new; > > > > if ((s == NULL) || > > ((new = malloc (strlen(s) + 1)) == NULL) ) { > > return NULL; > > } > > > > strcpy (new, s); > > return new; > > } > > > > But xstrdup() can't do so: > > > > char *xstrdup(const char *s) > > { > > char *p = strdup(s); > > > > if (!p) > > panic("ERROR: out of memory\n"); > > return p; > > } > > > > Can I just change first xstrdup() to strdup() in my patch? > > Why not add a NULL pointer check to xstrdup? This will be duplication of code. strdup() inside xstrdup() check it too. > > > > E.g. > > > > - gled->led.name = xstrdup(of_get_property(child, "label", NULL)); > > + gled->led.name = strdup(of_get_property(child, "label", NULL)); > > if (!gled->led.name) > > gled->led.name = xstrdup(child->name); > > I think this code will raise questions. When reading this code one could > easily ask why stdrup is used in the first place and xstrdup in the > second. A year ahead even we may not know the answer without thinking > about it ;) > > Sascha > > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- -- Best regards, Antony Pavlov _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] led: try to get LED's label from the 'label' property 2014-07-28 6:22 ` Antony Pavlov 2014-07-28 6:29 ` Sascha Hauer @ 2014-07-28 6:34 ` Sebastian Hesselbarth 2014-07-29 5:49 ` Antony Pavlov 1 sibling, 1 reply; 7+ messages in thread From: Sebastian Hesselbarth @ 2014-07-28 6:34 UTC (permalink / raw) To: Antony Pavlov, Sascha Hauer; +Cc: barebox On 07/28/2014 08:22 AM, Antony Pavlov wrote: > On Mon, 28 Jul 2014 07:49:17 +0200 > Sascha Hauer <s.hauer@pengutronix.de> wrote: > >> On Sun, Jul 27, 2014 at 12:35:52PM +0400, Antony Pavlov wrote: >>> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> >>> --- >>> drivers/led/led-gpio.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/led/led-gpio.c b/drivers/led/led-gpio.c >>> index 7bb3b49..cddac08 100644 >>> --- a/drivers/led/led-gpio.c >>> +++ b/drivers/led/led-gpio.c >>> @@ -212,7 +212,9 @@ static int led_gpio_of_probe(struct device_d *dev) >>> continue; >>> >>> gled = xzalloc(sizeof(*gled)); >>> - gled->led.name = xstrdup(child->name); >>> + gled->led.name = xstrdup(of_get_property(child, "label", NULL)); >> >> When the property doesn't exist you pass NULL to xstrdup. This will >> crash. > > My bad. I have seen barebox strdup() realization but missed xstrdup() realization. > > Barebox' strdup() can handle NULL argument: > > char * strdup(const char *s) > { > char *new; > > if ((s == NULL) || > ((new = malloc (strlen(s) + 1)) == NULL) ) { > return NULL; > } > > strcpy (new, s); > return new; > } > > But xstrdup() can't do so: > > char *xstrdup(const char *s) > { > char *p = strdup(s); > > if (!p) > panic("ERROR: out of memory\n"); > return p; > } > > Can I just change first xstrdup() to strdup() in my patch? > > E.g. > > - gled->led.name = xstrdup(of_get_property(child, "label", NULL)); > + gled->led.name = strdup(of_get_property(child, "label", NULL)); > if (!gled->led.name) > gled->led.name = xstrdup(child->name); > >> You'll have to add a check or make xstrdup tolerate NULL >> pointers. >> Correct code sequence would probably be: const char *label; if (of_property_read_string(child, "label", &label)) gled->led.name = xstrdup(label); else gled->led.name = xstrdup(child->name); or const char *label = child->name; of_property_read_string(child, "label", &label); gled->led.name = xstrdup(label); Sebastian >>> + if (!gled->led.name) >>> + gled->led.name = xstrdup(child->name); >>> gled->gpio = gpio; >>> gled->active_low = (flags & OF_GPIO_ACTIVE_LOW) ? 1 : 0; >>> >>> -- >>> 2.0.1 >>> >>> >>> _______________________________________________ >>> barebox mailing list >>> barebox@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/barebox >>> >> >> -- >> Pengutronix e.K. | | >> Industrial Linux Solutions | http://www.pengutronix.de/ | >> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] led: try to get LED's label from the 'label' property 2014-07-28 6:34 ` Sebastian Hesselbarth @ 2014-07-29 5:49 ` Antony Pavlov 0 siblings, 0 replies; 7+ messages in thread From: Antony Pavlov @ 2014-07-29 5:49 UTC (permalink / raw) To: Sebastian Hesselbarth; +Cc: barebox On Mon, 28 Jul 2014 08:34:43 +0200 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > On 07/28/2014 08:22 AM, Antony Pavlov wrote: > > On Mon, 28 Jul 2014 07:49:17 +0200 > > Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > >> On Sun, Jul 27, 2014 at 12:35:52PM +0400, Antony Pavlov wrote: > >>> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> > >>> --- > >>> drivers/led/led-gpio.c | 4 +++- > >>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/led/led-gpio.c b/drivers/led/led-gpio.c > >>> index 7bb3b49..cddac08 100644 > >>> --- a/drivers/led/led-gpio.c > >>> +++ b/drivers/led/led-gpio.c > >>> @@ -212,7 +212,9 @@ static int led_gpio_of_probe(struct device_d *dev) > >>> continue; > >>> > >>> gled = xzalloc(sizeof(*gled)); > >>> - gled->led.name = xstrdup(child->name); > >>> + gled->led.name = xstrdup(of_get_property(child, "label", NULL)); > >> > >> When the property doesn't exist you pass NULL to xstrdup. This will > >> crash. > > > > My bad. I have seen barebox strdup() realization but missed xstrdup() realization. > > > > Barebox' strdup() can handle NULL argument: > > > > char * strdup(const char *s) > > { > > char *new; > > > > if ((s == NULL) || > > ((new = malloc (strlen(s) + 1)) == NULL) ) { > > return NULL; > > } > > > > strcpy (new, s); > > return new; > > } > > > > But xstrdup() can't do so: > > > > char *xstrdup(const char *s) > > { > > char *p = strdup(s); > > > > if (!p) > > panic("ERROR: out of memory\n"); > > return p; > > } > > > > Can I just change first xstrdup() to strdup() in my patch? > > > > E.g. > > > > - gled->led.name = xstrdup(of_get_property(child, "label", NULL)); > > + gled->led.name = strdup(of_get_property(child, "label", NULL)); > > if (!gled->led.name) > > gled->led.name = xstrdup(child->name); > > > >> You'll have to add a check or make xstrdup tolerate NULL > >> pointers. > >> > > Correct code sequence would probably be: > > const char *label; > > if (of_property_read_string(child, "label", &label)) > gled->led.name = xstrdup(label); > else > gled->led.name = xstrdup(child->name); Hmm. of_property_read_string() returns 0 on success... > > or > > const char *label = child->name; > > of_property_read_string(child, "label", &label); > gled->led.name = xstrdup(label); > > Sebastian > > >>> + if (!gled->led.name) > >>> + gled->led.name = xstrdup(child->name); > >>> gled->gpio = gpio; > >>> gled->active_low = (flags & OF_GPIO_ACTIVE_LOW) ? 1 : 0; > >>> > >>> -- > >>> 2.0.1 > >>> > >>> > >>> _______________________________________________ > >>> barebox mailing list > >>> barebox@lists.infradead.org > >>> http://lists.infradead.org/mailman/listinfo/barebox > >>> > >> > >> -- > >> Pengutronix e.K. | | > >> Industrial Linux Solutions | http://www.pengutronix.de/ | > >> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > > > > -- -- Best regards, Antony Pavlov _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-07-29 5:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-07-27 8:35 [PATCH] led: try to get LED's label from the 'label' property Antony Pavlov 2014-07-28 5:49 ` Sascha Hauer 2014-07-28 6:22 ` Antony Pavlov 2014-07-28 6:29 ` Sascha Hauer 2014-07-28 7:02 ` Antony Pavlov 2014-07-28 6:34 ` Sebastian Hesselbarth 2014-07-29 5:49 ` Antony Pavlov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox