* [PATCH] [SCSI] fusion: ensure NUL-termination of MptCallbacksName elements
@ 2011-08-25 12:44 Ferenc Wagner
2011-08-25 12:44 ` [PATCH v2] SCSI: " Ferenc Wagner
2011-08-25 21:59 ` [PATCH] [SCSI] " Andrew Morton
0 siblings, 2 replies; 5+ messages in thread
From: Ferenc Wagner @ 2011-08-25 12:44 UTC (permalink / raw)
To: Kashyap, Desai, support, DL-MPTFusionLinux, linux-scsi,
linux-kernel
Signed-off-by: Ferenc Wagner <wferi@niif.hu>
---
drivers/message/fusion/mptbase.c | 5 ++---
drivers/message/fusion/mptbase.h | 1 +
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 7956a10..eb906e2 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -113,7 +113,7 @@ module_param(mpt_fwfault_debug, int, 0600);
MODULE_PARM_DESC(mpt_fwfault_debug,
"Enable detection of Firmware fault and halt Firmware on fault - (default=0)");
-static char MptCallbacksName[MPT_MAX_PROTOCOL_DRIVERS][50];
+static char MptCallbacksName[MPT_MAX_PROTOCOL_DRIVERS][MPT_MAX_CALLBACKNAME_LEN+1];
#ifdef MFCNT
static int mfcounter = 0;
@@ -656,8 +656,7 @@ mpt_register(MPT_CALLBACK cbfunc, MPT_DRIVER_CLASS dclass, char *func_name)
MptDriverClass[cb_idx] = dclass;
MptEvHandlers[cb_idx] = NULL;
last_drv_idx = cb_idx;
- memcpy(MptCallbacksName[cb_idx], func_name,
- strlen(func_name) > 50 ? 50 : strlen(func_name));
+ strlcpy(MptCallbacksName[cb_idx], func_name, MPT_MAX_CALLBACKNAME_LEN+1);
break;
}
}
diff --git a/drivers/message/fusion/mptbase.h b/drivers/message/fusion/mptbase.h
index fe90233..c10416d 100644
--- a/drivers/message/fusion/mptbase.h
+++ b/drivers/message/fusion/mptbase.h
@@ -89,6 +89,7 @@
*/
#define MPT_MAX_ADAPTERS 18
#define MPT_MAX_PROTOCOL_DRIVERS 16
+#define MPT_MAX_CALLBACKNAME_LEN 49
#define MPT_MAX_BUS 1 /* Do not change */
#define MPT_MAX_FC_DEVICES 255
#define MPT_MAX_SCSI_DEVICES 16
--
1.6.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] SCSI: fusion: ensure NUL-termination of MptCallbacksName elements
2011-08-25 12:44 [PATCH] [SCSI] fusion: ensure NUL-termination of MptCallbacksName elements Ferenc Wagner
@ 2011-08-25 12:44 ` Ferenc Wagner
2011-11-28 7:02 ` Nandigama, Nagalakshmi
2011-08-25 21:59 ` [PATCH] [SCSI] " Andrew Morton
1 sibling, 1 reply; 5+ messages in thread
From: Ferenc Wagner @ 2011-08-25 12:44 UTC (permalink / raw)
To: Andrew Morton
Cc: Kashyap, Desai, support, DL-MPTFusionLinux, linux-scsi,
linux-kernel
Using strlcpy instead of memcpy makes the strlen() calls superfluous
and also ensures zero-termination of the copied string.
At the same time get rid of the magic constant 50.
Actually, I can't see why copying the callback name is necessary
instead of simply storing a pointer to it (just like to the callback
function itself), but that goes beyond the scope of this fix.
Signed-off-by: Ferenc Wagner <wferi@niif.hu>
---
drivers/message/fusion/mptbase.c | 7 ++++---
drivers/message/fusion/mptbase.h | 1 +
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index e9c6a60..a7dc467 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -115,7 +115,8 @@ module_param(mpt_fwfault_debug, int, 0600);
MODULE_PARM_DESC(mpt_fwfault_debug,
"Enable detection of Firmware fault and halt Firmware on fault - (default=0)");
-static char MptCallbacksName[MPT_MAX_PROTOCOL_DRIVERS][50];
+static char MptCallbacksName[MPT_MAX_PROTOCOL_DRIVERS]
+ [MPT_MAX_CALLBACKNAME_LEN+1];
#ifdef MFCNT
static int mfcounter = 0;
@@ -717,8 +718,8 @@ mpt_register(MPT_CALLBACK cbfunc, MPT_DRIVER_CLASS dclass, char *func_name)
MptDriverClass[cb_idx] = dclass;
MptEvHandlers[cb_idx] = NULL;
last_drv_idx = cb_idx;
- memcpy(MptCallbacksName[cb_idx], func_name,
- strlen(func_name) > 50 ? 50 : strlen(func_name));
+ strlcpy(MptCallbacksName[cb_idx], func_name,
+ MPT_MAX_CALLBACKNAME_LEN+1);
break;
}
}
diff --git a/drivers/message/fusion/mptbase.h b/drivers/message/fusion/mptbase.h
index b4d24dc..76c05bc 100644
--- a/drivers/message/fusion/mptbase.h
+++ b/drivers/message/fusion/mptbase.h
@@ -89,6 +89,7 @@
*/
#define MPT_MAX_ADAPTERS 18
#define MPT_MAX_PROTOCOL_DRIVERS 16
+#define MPT_MAX_CALLBACKNAME_LEN 49
#define MPT_MAX_BUS 1 /* Do not change */
#define MPT_MAX_FC_DEVICES 255
#define MPT_MAX_SCSI_DEVICES 16
--
1.6.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [PATCH v2] SCSI: fusion: ensure NUL-termination of MptCallbacksName elements
2011-08-25 12:44 ` [PATCH v2] SCSI: " Ferenc Wagner
@ 2011-11-28 7:02 ` Nandigama, Nagalakshmi
0 siblings, 0 replies; 5+ messages in thread
From: Nandigama, Nagalakshmi @ 2011-11-28 7:02 UTC (permalink / raw)
To: Ferenc Wagner, Andrew Morton
Cc: Desai, Kashyap, Support, DL-MPT Fusion Linux,
linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
The patch seem to be fine. Please consider this patch as an Acked by me.
Regards,
Nagalakshmi
-----Original Message-----
From: Ferenc Wagner [mailto:wferi@niif.hu]
Sent: Thursday, August 25, 2011 6:15 PM
To: Andrew Morton
Cc: Desai, Kashyap; Support; DL-MPT Fusion Linux; linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] SCSI: fusion: ensure NUL-termination of MptCallbacksName elements
Using strlcpy instead of memcpy makes the strlen() calls superfluous
and also ensures zero-termination of the copied string.
At the same time get rid of the magic constant 50.
Actually, I can't see why copying the callback name is necessary
instead of simply storing a pointer to it (just like to the callback
function itself), but that goes beyond the scope of this fix.
Signed-off-by: Ferenc Wagner <wferi@niif.hu>
---
drivers/message/fusion/mptbase.c | 7 ++++---
drivers/message/fusion/mptbase.h | 1 +
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index e9c6a60..a7dc467 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -115,7 +115,8 @@ module_param(mpt_fwfault_debug, int, 0600);
MODULE_PARM_DESC(mpt_fwfault_debug,
"Enable detection of Firmware fault and halt Firmware on fault - (default=0)");
-static char MptCallbacksName[MPT_MAX_PROTOCOL_DRIVERS][50];
+static char MptCallbacksName[MPT_MAX_PROTOCOL_DRIVERS]
+ [MPT_MAX_CALLBACKNAME_LEN+1];
#ifdef MFCNT
static int mfcounter = 0;
@@ -717,8 +718,8 @@ mpt_register(MPT_CALLBACK cbfunc, MPT_DRIVER_CLASS dclass, char *func_name)
MptDriverClass[cb_idx] = dclass;
MptEvHandlers[cb_idx] = NULL;
last_drv_idx = cb_idx;
- memcpy(MptCallbacksName[cb_idx], func_name,
- strlen(func_name) > 50 ? 50 : strlen(func_name));
+ strlcpy(MptCallbacksName[cb_idx], func_name,
+ MPT_MAX_CALLBACKNAME_LEN+1);
break;
}
}
diff --git a/drivers/message/fusion/mptbase.h b/drivers/message/fusion/mptbase.h
index b4d24dc..76c05bc 100644
--- a/drivers/message/fusion/mptbase.h
+++ b/drivers/message/fusion/mptbase.h
@@ -89,6 +89,7 @@
*/
#define MPT_MAX_ADAPTERS 18
#define MPT_MAX_PROTOCOL_DRIVERS 16
+#define MPT_MAX_CALLBACKNAME_LEN 49
#define MPT_MAX_BUS 1 /* Do not change */
#define MPT_MAX_FC_DEVICES 255
#define MPT_MAX_SCSI_DEVICES 16
--
1.6.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] [SCSI] fusion: ensure NUL-termination of MptCallbacksName elements
2011-08-25 12:44 [PATCH] [SCSI] fusion: ensure NUL-termination of MptCallbacksName elements Ferenc Wagner
2011-08-25 12:44 ` [PATCH v2] SCSI: " Ferenc Wagner
@ 2011-08-25 21:59 ` Andrew Morton
2011-08-26 17:01 ` Ferenc Wagner
1 sibling, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2011-08-25 21:59 UTC (permalink / raw)
To: Ferenc Wagner
Cc: Kashyap, Desai, support, DL-MPTFusionLinux, linux-scsi,
linux-kernel
On Thu, 25 Aug 2011 14:44:57 +0200
Ferenc Wagner <wferi@niif.hu> wrote:
> Signed-off-by: Ferenc Wagner <wferi@niif.hu>
The lack of a changelog is a hint that the patch needed a changelog!
>
> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
> index 7956a10..eb906e2 100644
> --- a/drivers/message/fusion/mptbase.c
> +++ b/drivers/message/fusion/mptbase.c
> @@ -113,7 +113,7 @@ module_param(mpt_fwfault_debug, int, 0600);
> MODULE_PARM_DESC(mpt_fwfault_debug,
> "Enable detection of Firmware fault and halt Firmware on fault - (default=0)");
>
> -static char MptCallbacksName[MPT_MAX_PROTOCOL_DRIVERS][50];
> +static char MptCallbacksName[MPT_MAX_PROTOCOL_DRIVERS][MPT_MAX_CALLBACKNAME_LEN+1];
>
> #ifdef MFCNT
> static int mfcounter = 0;
> @@ -656,8 +656,7 @@ mpt_register(MPT_CALLBACK cbfunc, MPT_DRIVER_CLASS dclass, char *func_name)
> MptDriverClass[cb_idx] = dclass;
> MptEvHandlers[cb_idx] = NULL;
> last_drv_idx = cb_idx;
> - memcpy(MptCallbacksName[cb_idx], func_name,
> - strlen(func_name) > 50 ? 50 : strlen(func_name));
> + strlcpy(MptCallbacksName[cb_idx], func_name, MPT_MAX_CALLBACKNAME_LEN+1);
> break;
> }
> }
> diff --git a/drivers/message/fusion/mptbase.h b/drivers/message/fusion/mptbase.h
> index fe90233..c10416d 100644
> --- a/drivers/message/fusion/mptbase.h
> +++ b/drivers/message/fusion/mptbase.h
> @@ -89,6 +89,7 @@
> */
> #define MPT_MAX_ADAPTERS 18
> #define MPT_MAX_PROTOCOL_DRIVERS 16
> +#define MPT_MAX_CALLBACKNAME_LEN 49
> #define MPT_MAX_BUS 1 /* Do not change */
> #define MPT_MAX_FC_DEVICES 255
> #define MPT_MAX_SCSI_DEVICES 16
Does the patch fix some real-world observed problem? If so, please
fully describe it.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [SCSI] fusion: ensure NUL-termination of MptCallbacksName elements
2011-08-25 21:59 ` [PATCH] [SCSI] " Andrew Morton
@ 2011-08-26 17:01 ` Ferenc Wagner
0 siblings, 0 replies; 5+ messages in thread
From: Ferenc Wagner @ 2011-08-26 17:01 UTC (permalink / raw)
To: Andrew Morton
Cc: Kashyap, Desai, support, DL-MPTFusionLinux, linux-scsi,
linux-kernel
Andrew Morton <akpm@linux-foundation.org> writes:
> On Thu, 25 Aug 2011 14:44:57 +0200
> Ferenc Wagner <wferi@niif.hu> wrote:
>
>> Signed-off-by: Ferenc Wagner <wferi@niif.hu>
>
> The lack of a changelog is a hint that the patch needed a changelog!
Sorry, I thought repeating the subject wasn't useful. I could also talk
a little about removing a magic number, but that's about it...
Concerning the checkpatch fixup patch, I decided that mptbase.c
contained overfull lines already, so I'd better keep these on a single
line. Of course I don't argue for this the least.
>> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
>> index 7956a10..eb906e2 100644
>> --- a/drivers/message/fusion/mptbase.c
>> +++ b/drivers/message/fusion/mptbase.c
>> @@ -113,7 +113,7 @@ module_param(mpt_fwfault_debug, int, 0600);
>> MODULE_PARM_DESC(mpt_fwfault_debug,
>> "Enable detection of Firmware fault and halt Firmware on fault - (default=0)");
>>
>> -static char MptCallbacksName[MPT_MAX_PROTOCOL_DRIVERS][50];
>> +static char MptCallbacksName[MPT_MAX_PROTOCOL_DRIVERS][MPT_MAX_CALLBACKNAME_LEN+1];
>>
>> #ifdef MFCNT
>> static int mfcounter = 0;
>> @@ -656,8 +656,7 @@ mpt_register(MPT_CALLBACK cbfunc, MPT_DRIVER_CLASS dclass, char *func_name)
>> MptDriverClass[cb_idx] = dclass;
>> MptEvHandlers[cb_idx] = NULL;
>> last_drv_idx = cb_idx;
>> - memcpy(MptCallbacksName[cb_idx], func_name,
>> - strlen(func_name) > 50 ? 50 : strlen(func_name));
>> + strlcpy(MptCallbacksName[cb_idx], func_name, MPT_MAX_CALLBACKNAME_LEN+1);
>> break;
>> }
>> }
>> diff --git a/drivers/message/fusion/mptbase.h b/drivers/message/fusion/mptbase.h
>> index fe90233..c10416d 100644
>> --- a/drivers/message/fusion/mptbase.h
>> +++ b/drivers/message/fusion/mptbase.h
>> @@ -89,6 +89,7 @@
>> */
>> #define MPT_MAX_ADAPTERS 18
>> #define MPT_MAX_PROTOCOL_DRIVERS 16
>> +#define MPT_MAX_CALLBACKNAME_LEN 49
>> #define MPT_MAX_BUS 1 /* Do not change */
>> #define MPT_MAX_FC_DEVICES 255
>> #define MPT_MAX_SCSI_DEVICES 16
>
> Does the patch fix some real-world observed problem? If so, please
> fully describe it.
Not at all, I just stumbled upon this while pondering over
https://bugzilla.kernel.org/show_bug.cgi?id=26692 and thought this could
be made better. Actually, I can't see why copying the callback name is
necessary instead of simply storing a pointer to it (just like to the
callback function itself), but that goes beyond my scope, I'm afraid.
I'll send a v2 of this patch in a couple of days with shorter lines and
longer description. Please all feel free to express opinions about the
name of the constant, the +1s and/or the spaces around the binop, etc. :)
--
Thanks,
Feri.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-11-28 7:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-25 12:44 [PATCH] [SCSI] fusion: ensure NUL-termination of MptCallbacksName elements Ferenc Wagner
2011-08-25 12:44 ` [PATCH v2] SCSI: " Ferenc Wagner
2011-11-28 7:02 ` Nandigama, Nagalakshmi
2011-08-25 21:59 ` [PATCH] [SCSI] " Andrew Morton
2011-08-26 17:01 ` Ferenc Wagner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox