Re: [patch] [SCSI] libosd: check for kzalloc() failure
Am 30.01.2013 08:06, schrieb Dan Carpenter:
There wasn't any error handling for this kzalloc().
Signed-off-by: Dan Carpenter dan.carpen...@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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [SCSI] libosd: check for kzalloc() failure
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.carpen...@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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [SCSI] libosd: check for kzalloc() failure
On Wed, Jan 30, 2013 at 10:57 AM, walter harms wha...@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.carpen...@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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [SCSI] libosd: check for kzalloc() failure
On Wed, Jan 30, 2013 at 9:06 AM, Dan Carpenter dan.carpen...@oracle.com wrote:
There wasn't any error handling for this kzalloc().
Signed-off-by: Dan Carpenter dan.carpen...@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);
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [SCSI] libosd: check for kzalloc() failure
Am 30.01.2013 10:51, schrieb Benny Halevy:
On Wed, Jan 30, 2013 at 10:57 AM, walter harms wha...@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.carpen...@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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [SCSI] libosd: check for kzalloc() failure
On Wed, Jan 30, 2013 at 3:00 PM, walter harms wha...@bfs.de wrote:
Am 30.01.2013 10:51, schrieb Benny Halevy:
On Wed, Jan 30, 2013 at 10:57 AM, walter harms wha...@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.carpen...@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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [SCSI] libosd: check for kzalloc() failure
Am 30.01.2013 14:40, schrieb Benny Halevy:
On Wed, Jan 30, 2013 at 3:00 PM, walter harms wha...@bfs.de wrote:
Am 30.01.2013 10:51, schrieb Benny Halevy:
On Wed, Jan 30, 2013 at 10:57 AM, walter harms wha...@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.carpen...@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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [SCSI] libosd: check for kzalloc() failure
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 -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [SCSI] libosd: check for kzalloc() failure
On 01/30/2013 09:06 AM, Dan Carpenter wrote:
There wasn't any error handling for this kzalloc().
ACK-by: Boaz Harrosh bharr...@panasas.com
James please queue for inclusion
Signed-off-by: Dan Carpenter dan.carpen...@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);
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
