* [patch] [SCSI] libosd: check for kzalloc() failure
@ 2013-01-30 7:06 Dan Carpenter
2013-01-30 8:15 ` walter harms
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Dan Carpenter @ 2013-01-30 7:06 UTC (permalink / raw)
To: Boaz Harrosh
Cc: Benny Halevy, James E.J. Bottomley, osd-dev, linux-scsi,
kernel-janitors
There wasn't any error handling for this kzalloc().
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index c06b8e5..d8293f2 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od,
odi->osdname_len = get_attrs[a].len;
/* Avoid NULL for memcmp optimization 0-length is good enough */
odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
+ if (!odi->osdname) {
+ ret = -ENOMEM;
+ goto out;
+ }
if (odi->osdname_len)
memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
OSD_INFO("OSD_NAME [%s]\n", odi->osdname);
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [patch] [SCSI] libosd: check for kzalloc() failure 2013-01-30 7:06 [patch] [SCSI] libosd: check for kzalloc() failure Dan Carpenter @ 2013-01-30 8:15 ` walter harms 2013-01-30 8:27 ` Dan Carpenter 2013-01-30 10:05 ` Benny Halevy 2013-01-30 15:56 ` Boaz Harrosh 2 siblings, 1 reply; 11+ messages in thread From: walter harms @ 2013-01-30 8:15 UTC (permalink / raw) To: Dan Carpenter Cc: Boaz Harrosh, Benny Halevy, James E.J. Bottomley, osd-dev, linux-scsi, kernel-janitors Am 30.01.2013 08:06, schrieb Dan Carpenter: > There wasn't any error handling for this kzalloc(). > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c > index c06b8e5..d8293f2 100644 > --- a/drivers/scsi/osd/osd_initiator.c > +++ b/drivers/scsi/osd/osd_initiator.c > @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od, > odi->osdname_len = get_attrs[a].len; > /* Avoid NULL for memcmp optimization 0-length is good enough */ > odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL); > + if (!odi->osdname) { > + ret = -ENOMEM; > + goto out; > + } > if (odi->osdname_len) > memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len); > OSD_INFO("OSD_NAME [%s]\n", odi->osdname); > -- this looks like strdup() ? re, wh ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure 2013-01-30 8:15 ` walter harms @ 2013-01-30 8:27 ` Dan Carpenter 2013-01-30 8:57 ` walter harms 0 siblings, 1 reply; 11+ messages in thread From: Dan Carpenter @ 2013-01-30 8:27 UTC (permalink / raw) To: walter harms Cc: Boaz Harrosh, Benny Halevy, James E.J. Bottomley, osd-dev, linux-scsi, kernel-janitors On Wed, Jan 30, 2013 at 09:15:43AM +0100, walter harms wrote: > > > Am 30.01.2013 08:06, schrieb Dan Carpenter: > > There wasn't any error handling for this kzalloc(). > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c > > index c06b8e5..d8293f2 100644 > > --- a/drivers/scsi/osd/osd_initiator.c > > +++ b/drivers/scsi/osd/osd_initiator.c > > @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od, > > odi->osdname_len = get_attrs[a].len; > > /* Avoid NULL for memcmp optimization 0-length is good enough */ > > odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL); > > + if (!odi->osdname) { > > + ret = -ENOMEM; > > + goto out; > > + } > > if (odi->osdname_len) > > memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len); > > OSD_INFO("OSD_NAME [%s]\n", odi->osdname); > > -- > > this looks like strdup() ? > Maybe? It's a funny thing going on with the NUL terminator and I don't understand what the comment is about. It appears that normally "get_attrs[a].val_ptr" is a NUL terminated string but "get_attrs[a].len" does not count the terminator. Odd. regards, dan carpenter ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure 2013-01-30 8:27 ` Dan Carpenter @ 2013-01-30 8:57 ` walter harms 2013-01-30 9:51 ` Benny Halevy 0 siblings, 1 reply; 11+ messages in thread From: walter harms @ 2013-01-30 8:57 UTC (permalink / raw) To: Dan Carpenter Cc: Boaz Harrosh, Benny Halevy, James E.J. Bottomley, osd-dev, linux-scsi, kernel-janitors Am 30.01.2013 09:27, schrieb Dan Carpenter: > On Wed, Jan 30, 2013 at 09:15:43AM +0100, walter harms wrote: >> >> >> Am 30.01.2013 08:06, schrieb Dan Carpenter: >>> There wasn't any error handling for this kzalloc(). >>> >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >>> >>> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c >>> index c06b8e5..d8293f2 100644 >>> --- a/drivers/scsi/osd/osd_initiator.c >>> +++ b/drivers/scsi/osd/osd_initiator.c >>> @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od, >>> odi->osdname_len = get_attrs[a].len; >>> /* Avoid NULL for memcmp optimization 0-length is good enough */ >>> odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL); >>> + if (!odi->osdname) { >>> + ret = -ENOMEM; >>> + goto out; >>> + } >>> if (odi->osdname_len) >>> memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len); >>> OSD_INFO("OSD_NAME [%s]\n", odi->osdname); >>> -- >> >> this looks like strdup() ? >> > > Maybe? It's a funny thing going on with the NUL terminator and I > don't understand what the comment is about. > > It appears that normally "get_attrs[a].val_ptr" is a NUL terminated > string but "get_attrs[a].len" does not count the terminator. > > Odd. > i have no clue what the programmer was thinking. if i read this correct osdname is u8 *osdname; so a simple strdup() or strndup() would be ok the comment seems to indicate that get_attrs[a].val_ptr could be NULL but where is the check ? Perhaps they are not using ascii here ? then a memdup(get_attrs[a].len) would be better. re, wh ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure 2013-01-30 8:57 ` walter harms @ 2013-01-30 9:51 ` Benny Halevy 2013-01-30 13:00 ` walter harms 0 siblings, 1 reply; 11+ messages in thread From: Benny Halevy @ 2013-01-30 9:51 UTC (permalink / raw) To: wharms Cc: Dan Carpenter, Boaz Harrosh, James E.J. Bottomley, osd-dev, linux-scsi, kernel-janitors On Wed, Jan 30, 2013 at 10:57 AM, walter harms <wharms@bfs.de> wrote: > > > Am 30.01.2013 09:27, schrieb Dan Carpenter: >> On Wed, Jan 30, 2013 at 09:15:43AM +0100, walter harms wrote: >>> >>> >>> Am 30.01.2013 08:06, schrieb Dan Carpenter: >>>> There wasn't any error handling for this kzalloc(). >>>> >>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >>>> >>>> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c >>>> index c06b8e5..d8293f2 100644 >>>> --- a/drivers/scsi/osd/osd_initiator.c >>>> +++ b/drivers/scsi/osd/osd_initiator.c >>>> @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od, >>>> odi->osdname_len = get_attrs[a].len; >>>> /* Avoid NULL for memcmp optimization 0-length is good enough */ >>>> odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL); >>>> + if (!odi->osdname) { >>>> + ret = -ENOMEM; >>>> + goto out; >>>> + } >>>> if (odi->osdname_len) >>>> memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len); >>>> OSD_INFO("OSD_NAME [%s]\n", odi->osdname); >>>> -- >>> >>> this looks like strdup() ? >>> >> >> Maybe? It's a funny thing going on with the NUL terminator and I >> don't understand what the comment is about. >> >> It appears that normally "get_attrs[a].val_ptr" is a NUL terminated >> string but "get_attrs[a].len" does not count the terminator. >> >> Odd. >> > i have no clue what the programmer was thinking. if i read this correct > osdname is u8 *osdname; so a simple strdup() or strndup() would be ok > the comment seems to indicate that get_attrs[a].val_ptr could be NULL > but where is the check ? > Perhaps they are not using ascii here ? then a memdup(get_attrs[a].len) > would be better. There are subtle differences between kstrdup or kmemdup and this implementation. kmemdup is problematic as get_attrs[a].val_ptr does not contain the NUL terminator In the following case: if get_attrs[a].len = 0 then get_attrs[a].val_ptr = NULL The end result should be odi->osdname_len = 0 odi->osdname = kzalloc(1, GFP_KERNEL); while with kstrdup, odi->osdname will end up being NULL as it's input arg "s" is NULL. Benny > > re, > wh > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure 2013-01-30 9:51 ` Benny Halevy @ 2013-01-30 13:00 ` walter harms 2013-01-30 13:40 ` Benny Halevy 0 siblings, 1 reply; 11+ messages in thread From: walter harms @ 2013-01-30 13:00 UTC (permalink / raw) To: Benny Halevy Cc: Dan Carpenter, Boaz Harrosh, James E.J. Bottomley, osd-dev, linux-scsi, kernel-janitors Am 30.01.2013 10:51, schrieb Benny Halevy: > On Wed, Jan 30, 2013 at 10:57 AM, walter harms <wharms@bfs.de> wrote: >> >> >> Am 30.01.2013 09:27, schrieb Dan Carpenter: >>> On Wed, Jan 30, 2013 at 09:15:43AM +0100, walter harms wrote: >>>> >>>> >>>> Am 30.01.2013 08:06, schrieb Dan Carpenter: >>>>> There wasn't any error handling for this kzalloc(). >>>>> >>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >>>>> >>>>> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c >>>>> index c06b8e5..d8293f2 100644 >>>>> --- a/drivers/scsi/osd/osd_initiator.c >>>>> +++ b/drivers/scsi/osd/osd_initiator.c >>>>> @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od, >>>>> odi->osdname_len = get_attrs[a].len; >>>>> /* Avoid NULL for memcmp optimization 0-length is good enough */ >>>>> odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL); >>>>> + if (!odi->osdname) { >>>>> + ret = -ENOMEM; >>>>> + goto out; >>>>> + } >>>>> if (odi->osdname_len) >>>>> memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len); >>>>> OSD_INFO("OSD_NAME [%s]\n", odi->osdname); >>>>> -- >>>> >>>> this looks like strdup() ? >>>> >>> >>> Maybe? It's a funny thing going on with the NUL terminator and I >>> don't understand what the comment is about. >>> >>> It appears that normally "get_attrs[a].val_ptr" is a NUL terminated >>> string but "get_attrs[a].len" does not count the terminator. >>> >>> Odd. >>> >> i have no clue what the programmer was thinking. if i read this correct >> osdname is u8 *osdname; so a simple strdup() or strndup() would be ok >> the comment seems to indicate that get_attrs[a].val_ptr could be NULL >> but where is the check ? >> Perhaps they are not using ascii here ? then a memdup(get_attrs[a].len) >> would be better. > > There are subtle differences between kstrdup or kmemdup and this implementation. > > kmemdup is problematic as get_attrs[a].val_ptr does not contain the > NUL terminator ok, i understand - but can we assume that we are talking ascii here ? > In the following case: > if get_attrs[a].len = 0 > then get_attrs[a].val_ptr = NULL > > The end result should be > odi->osdname_len = 0 > odi->osdname = kzalloc(1, GFP_KERNEL); > > while with kstrdup, odi->osdname will end up being NULL > as it's input arg "s" is NULL. > and you want the argument to be "" ? May i ask why ? kfree() can handle NULL and kprintf() (-family) also. re, wh > Benny > >> >> re, >> wh >> >> > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure 2013-01-30 13:00 ` walter harms @ 2013-01-30 13:40 ` Benny Halevy 2013-01-30 14:34 ` walter harms 0 siblings, 1 reply; 11+ messages in thread From: Benny Halevy @ 2013-01-30 13:40 UTC (permalink / raw) To: wharms Cc: Dan Carpenter, Boaz Harrosh, James E.J. Bottomley, osd-dev, linux-scsi, kernel-janitors On Wed, Jan 30, 2013 at 3:00 PM, walter harms <wharms@bfs.de> wrote: > > > Am 30.01.2013 10:51, schrieb Benny Halevy: >> On Wed, Jan 30, 2013 at 10:57 AM, walter harms <wharms@bfs.de> wrote: >>> >>> >>> Am 30.01.2013 09:27, schrieb Dan Carpenter: >>>> On Wed, Jan 30, 2013 at 09:15:43AM +0100, walter harms wrote: >>>>> >>>>> >>>>> Am 30.01.2013 08:06, schrieb Dan Carpenter: >>>>>> There wasn't any error handling for this kzalloc(). >>>>>> >>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >>>>>> >>>>>> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c >>>>>> index c06b8e5..d8293f2 100644 >>>>>> --- a/drivers/scsi/osd/osd_initiator.c >>>>>> +++ b/drivers/scsi/osd/osd_initiator.c >>>>>> @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od, >>>>>> odi->osdname_len = get_attrs[a].len; >>>>>> /* Avoid NULL for memcmp optimization 0-length is good enough */ >>>>>> odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL); >>>>>> + if (!odi->osdname) { >>>>>> + ret = -ENOMEM; >>>>>> + goto out; >>>>>> + } >>>>>> if (odi->osdname_len) >>>>>> memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len); >>>>>> OSD_INFO("OSD_NAME [%s]\n", odi->osdname); >>>>>> -- >>>>> >>>>> this looks like strdup() ? >>>>> >>>> >>>> Maybe? It's a funny thing going on with the NUL terminator and I >>>> don't understand what the comment is about. >>>> >>>> It appears that normally "get_attrs[a].val_ptr" is a NUL terminated >>>> string but "get_attrs[a].len" does not count the terminator. >>>> >>>> Odd. >>>> >>> i have no clue what the programmer was thinking. if i read this correct >>> osdname is u8 *osdname; so a simple strdup() or strndup() would be ok >>> the comment seems to indicate that get_attrs[a].val_ptr could be NULL >>> but where is the check ? >>> Perhaps they are not using ascii here ? then a memdup(get_attrs[a].len) >>> would be better. >> >> There are subtle differences between kstrdup or kmemdup and this implementation. >> >> kmemdup is problematic as get_attrs[a].val_ptr does not contain the >> NUL terminator > > ok, i understand - but can we assume that we are talking ascii here ? > No, it can be anything. UTF-8 is more likely but not guaranteed either. >> In the following case: >> if get_attrs[a].len = 0 >> then get_attrs[a].val_ptr = NULL >> >> The end result should be >> odi->osdname_len = 0 >> odi->osdname = kzalloc(1, GFP_KERNEL); >> >> while with kstrdup, odi->osdname will end up being NULL >> as it's input arg "s" is NULL. >> > > and you want the argument to be "" ? > May i ask why ? kfree() can handle NULL and kprintf() (-family) also. It was Boaz' decision at the time to simplify internal tests like _the_same_or_null in drivers/scsi/osd/osd_uld.c that doesn't check for NULL Nothing spectacular :) Benny > > re, > wh > > >> Benny >> >>> >>> re, >>> wh >>> >>> >> >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure 2013-01-30 13:40 ` Benny Halevy @ 2013-01-30 14:34 ` walter harms 2013-01-30 15:51 ` Boaz Harrosh 0 siblings, 1 reply; 11+ messages in thread From: walter harms @ 2013-01-30 14:34 UTC (permalink / raw) To: Benny Halevy Cc: Dan Carpenter, Boaz Harrosh, James E.J. Bottomley, osd-dev, linux-scsi, kernel-janitors Am 30.01.2013 14:40, schrieb Benny Halevy: > On Wed, Jan 30, 2013 at 3:00 PM, walter harms <wharms@bfs.de> wrote: >> >> >> Am 30.01.2013 10:51, schrieb Benny Halevy: >>> On Wed, Jan 30, 2013 at 10:57 AM, walter harms <wharms@bfs.de> wrote: >>>> >>>> >>>> Am 30.01.2013 09:27, schrieb Dan Carpenter: >>>>> On Wed, Jan 30, 2013 at 09:15:43AM +0100, walter harms wrote: >>>>>> >>>>>> >>>>>> Am 30.01.2013 08:06, schrieb Dan Carpenter: >>>>>>> There wasn't any error handling for this kzalloc(). >>>>>>> >>>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >>>>>>> >>>>>>> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c >>>>>>> index c06b8e5..d8293f2 100644 >>>>>>> --- a/drivers/scsi/osd/osd_initiator.c >>>>>>> +++ b/drivers/scsi/osd/osd_initiator.c >>>>>>> @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od, >>>>>>> odi->osdname_len = get_attrs[a].len; >>>>>>> /* Avoid NULL for memcmp optimization 0-length is good enough */ >>>>>>> odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL); >>>>>>> + if (!odi->osdname) { >>>>>>> + ret = -ENOMEM; >>>>>>> + goto out; >>>>>>> + } >>>>>>> if (odi->osdname_len) >>>>>>> memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len); >>>>>>> OSD_INFO("OSD_NAME [%s]\n", odi->osdname); >>>>>>> -- >>>>>> >>>>>> this looks like strdup() ? >>>>>> >>>>> >>>>> Maybe? It's a funny thing going on with the NUL terminator and I >>>>> don't understand what the comment is about. >>>>> >>>>> It appears that normally "get_attrs[a].val_ptr" is a NUL terminated >>>>> string but "get_attrs[a].len" does not count the terminator. >>>>> >>>>> Odd. >>>>> >>>> i have no clue what the programmer was thinking. if i read this correct >>>> osdname is u8 *osdname; so a simple strdup() or strndup() would be ok >>>> the comment seems to indicate that get_attrs[a].val_ptr could be NULL >>>> but where is the check ? >>>> Perhaps they are not using ascii here ? then a memdup(get_attrs[a].len) >>>> would be better. >>> >>> There are subtle differences between kstrdup or kmemdup and this implementation. >>> >>> kmemdup is problematic as get_attrs[a].val_ptr does not contain the >>> NUL terminator >> >> ok, i understand - but can we assume that we are talking ascii here ? >> > > No, it can be anything. UTF-8 is more likely but not guaranteed either. > I start to see the complexity of the situation. Would you mind to add the comment "it can be anything. UTF-8 is more likely but not guaranteed either" ? For now using a pascal-string seems the best solution but it should be warned that get_attrs[a].val_ptr is NOT a c-string and therefore can not be used with the printf-family (i guess the situation will become more clear in future) I have no clue why you need that, using c-strings makes life more easy in the last minute a sprintf(buf,"%u") will save the day ;) >>> In the following case: >>> if get_attrs[a].len = 0 >>> then get_attrs[a].val_ptr = NULL >>> >>> The end result should be >>> odi->osdname_len = 0 >>> odi->osdname = kzalloc(1, GFP_KERNEL); >>> >>> while with kstrdup, odi->osdname will end up being NULL >>> as it's input arg "s" is NULL. >>> >> >> and you want the argument to be "" ? >> May i ask why ? kfree() can handle NULL and kprintf() (-family) also. > > It was Boaz' decision at the time to simplify internal tests > like _the_same_or_null in drivers/scsi/osd/osd_uld.c > that doesn't check for NULL > It look clever to add the NULL test here. Noone reviewing the code will understand that. (Rule of least surprise) just my 2 cents, re, wh > Nothing spectacular :) > > Benny > >> >> re, >> wh >> >> >>> Benny >>> >>>> >>>> re, >>>> wh >>>> >>>> >>> >>> > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure 2013-01-30 14:34 ` walter harms @ 2013-01-30 15:51 ` Boaz Harrosh 0 siblings, 0 replies; 11+ messages in thread From: Boaz Harrosh @ 2013-01-30 15:51 UTC (permalink / raw) To: wharms Cc: Benny Halevy, Dan Carpenter, James E.J. Bottomley, osd-dev, linux-scsi, kernel-janitors On 01/30/2013 04:34 PM, walter harms wrote: > <> > I start to see the complexity of the situation. Would you mind to add > the comment "it can be anything. UTF-8 is more likely but not guaranteed either" ? > For now using a pascal-string seems the best solution but it should be warned > that get_attrs[a].val_ptr is NOT a c-string and therefore can not be used with > the printf-family (i guess the situation will become more clear in future) > OK, So... The long story The STD says that osdname can be *anything* binary or otherwise, and of *any* length. And is used to uniquely identify the OSD-device/partition in a cluster. We decided that if so we would stick an 40 char UUID in it, generated by a uuidgen and all the external utilities and surrounding code forces it, and treat it as a c-string. But this code here in the core cannot make that assumption and still support the STD. On the other hand we did want the osdname to be printable in traces and messages because it is such an important identifier. So I have decided to sacrifice an extra char in-memory to carry a \NUL and safely stick it into printk(s). Those (none existent) OSD devices that will put unprintable characters in here will still function fine, but will look real scary in printk(s). Please note that the one that sets policy is the osd-target vendor. (And they all currently use my code base) So save the kzalloc check this code (tested) is safe and will show strings when present, but will gracefully show ugly things but still work when not. > I have no clue why you need that, using c-strings makes life more easy in the > last minute a sprintf(buf,"%u") will save the day ;) > Actually it is very funny, because just recently (last week) I have discovered something that eliminates all those funny business. printf("%*s", odi->osdname, odi->osdname_len); The "*" will instruct c to expect an extra variable following %s which is the max_length of %s. This is exactly for pascal strings and the such like above. So I added a TODO to clean that a bit by always printing with "%*s" >> > It look clever to add the NULL test here. > Noone reviewing the code will understand that. > (Rule of least surprise) > Thanks for looking, I agree it needs a fat comment, I'll do that when I'll convert to above system. Thanks for looking, That code is really old and never had any extra eyes on it. > just my 2 cents, > re, > wh > Cheers Boaz ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure 2013-01-30 7:06 [patch] [SCSI] libosd: check for kzalloc() failure Dan Carpenter 2013-01-30 8:15 ` walter harms @ 2013-01-30 10:05 ` Benny Halevy 2013-01-30 15:56 ` Boaz Harrosh 2 siblings, 0 replies; 11+ messages in thread From: Benny Halevy @ 2013-01-30 10:05 UTC (permalink / raw) To: Dan Carpenter Cc: Boaz Harrosh, James E.J. Bottomley, osd-dev, linux-scsi, kernel-janitors On Wed, Jan 30, 2013 at 9:06 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > There wasn't any error handling for this kzalloc(). > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c > index c06b8e5..d8293f2 100644 > --- a/drivers/scsi/osd/osd_initiator.c > +++ b/drivers/scsi/osd/osd_initiator.c > @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od, > odi->osdname_len = get_attrs[a].len; > /* Avoid NULL for memcmp optimization 0-length is good enough */ > odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL); > + if (!odi->osdname) { > + ret = -ENOMEM; > + goto out; > + } good catch! Benny > if (odi->osdname_len) > memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len); > OSD_INFO("OSD_NAME [%s]\n", odi->osdname); ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure 2013-01-30 7:06 [patch] [SCSI] libosd: check for kzalloc() failure Dan Carpenter 2013-01-30 8:15 ` walter harms 2013-01-30 10:05 ` Benny Halevy @ 2013-01-30 15:56 ` Boaz Harrosh 2 siblings, 0 replies; 11+ messages in thread From: Boaz Harrosh @ 2013-01-30 15:56 UTC (permalink / raw) To: Dan Carpenter, James E.J. Bottomley Cc: Benny Halevy, osd-dev, linux-scsi, kernel-janitors On 01/30/2013 09:06 AM, Dan Carpenter wrote: > There wasn't any error handling for this kzalloc(). > ACK-by: Boaz Harrosh <bharrosh@panasas.com> James please queue for inclusion > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Thanks Dan > > diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c > index c06b8e5..d8293f2 100644 > --- a/drivers/scsi/osd/osd_initiator.c > +++ b/drivers/scsi/osd/osd_initiator.c > @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od, > odi->osdname_len = get_attrs[a].len; > /* Avoid NULL for memcmp optimization 0-length is good enough */ > odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL); > + if (!odi->osdname) { > + ret = -ENOMEM; > + goto out; > + } > if (odi->osdname_len) > memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len); > OSD_INFO("OSD_NAME [%s]\n", odi->osdname); > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-01-30 15:56 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-30 7:06 [patch] [SCSI] libosd: check for kzalloc() failure Dan Carpenter 2013-01-30 8:15 ` walter harms 2013-01-30 8:27 ` Dan Carpenter 2013-01-30 8:57 ` walter harms 2013-01-30 9:51 ` Benny Halevy 2013-01-30 13:00 ` walter harms 2013-01-30 13:40 ` Benny Halevy 2013-01-30 14:34 ` walter harms 2013-01-30 15:51 ` Boaz Harrosh 2013-01-30 10:05 ` Benny Halevy 2013-01-30 15:56 ` Boaz Harrosh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox