* [PATCH 1/1] module: fix sprintf format specifier in param_get_byte() @ 2013-08-06 21:12 Christoph Jaeger 2013-08-06 23:39 ` Joe Perches 2013-08-07 6:43 ` Rusty Russell 0 siblings, 2 replies; 5+ messages in thread From: Christoph Jaeger @ 2013-08-06 21:12 UTC (permalink / raw) To: Rusty Russell; +Cc: linux-kernel, Christoph Jaeger In param_get_byte(), to which the macro STANDARD_PARAM_DEF(byte, ...) expands, "%c" is used to print an unsigned char. So it gets printed as a character what is not intended here. Use "%hhu" instead. Signed-off-by: Christoph Jaeger <christophjaeger@linux.com> --- kernel/params.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/params.c b/kernel/params.c index 440e65d..59f7ac7 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -252,7 +252,7 @@ int parse_args(const char *doing, EXPORT_SYMBOL(param_ops_##name) -STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, strict_strtoul); +STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", unsigned long, strict_strtoul); STANDARD_PARAM_DEF(short, short, "%hi", long, strict_strtol); STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, strict_strtoul); STANDARD_PARAM_DEF(int, int, "%i", long, strict_strtol); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] module: fix sprintf format specifier in param_get_byte() 2013-08-06 21:12 [PATCH 1/1] module: fix sprintf format specifier in param_get_byte() Christoph Jaeger @ 2013-08-06 23:39 ` Joe Perches 2013-08-07 6:43 ` Rusty Russell 1 sibling, 0 replies; 5+ messages in thread From: Joe Perches @ 2013-08-06 23:39 UTC (permalink / raw) To: Christoph Jaeger; +Cc: Rusty Russell, linux-kernel On Tue, 2013-08-06 at 23:12 +0200, Christoph Jaeger wrote: > In param_get_byte(), to which the macro STANDARD_PARAM_DEF(byte, ...) expands, > "%c" is used to print an unsigned char. So it gets printed as a character what > is not intended here. Use "%hhu" instead. > > Signed-off-by: Christoph Jaeger <christophjaeger@linux.com> > --- > kernel/params.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/params.c b/kernel/params.c > index 440e65d..59f7ac7 100644 > --- a/kernel/params.c > +++ b/kernel/params.c > @@ -252,7 +252,7 @@ int parse_args(const char *doing, > EXPORT_SYMBOL(param_ops_##name) > > > -STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, strict_strtoul); > +STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", unsigned long, strict_strtoul); > STANDARD_PARAM_DEF(short, short, "%hi", long, strict_strtol); > STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, strict_strtoul); I don't see the point of using "%hh[ui]" and "%h[ui]". These are promoted to int/unsigned int for the sprintf anyway. I'd just use %d and %u instead. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] module: fix sprintf format specifier in param_get_byte() 2013-08-06 21:12 [PATCH 1/1] module: fix sprintf format specifier in param_get_byte() Christoph Jaeger 2013-08-06 23:39 ` Joe Perches @ 2013-08-07 6:43 ` Rusty Russell 2013-08-07 11:31 ` Michal Nazarewicz 2013-08-07 16:45 ` Jon Mason 1 sibling, 2 replies; 5+ messages in thread From: Rusty Russell @ 2013-08-07 6:43 UTC (permalink / raw) To: Christoph Jaeger Cc: linux-kernel, Dmitry Tarnyagin, Jon Mason, tech.support, David Woodhouse, Michal Nazarewicz Christoph Jaeger <christophjaeger@linux.com> writes: > In param_get_byte(), to which the macro STANDARD_PARAM_DEF(byte, ...) expands, > "%c" is used to print an unsigned char. So it gets printed as a character what > is not intended here. Use "%hhu" instead. > > Signed-off-by: Christoph Jaeger <christophjaeger@linux.com> Nice patch. Unfortunately, there are several users of this already: drivers/net/wireless/cw1200/main.c:50:module_param_array_named(macaddr, cw1200_mac_template, byte, NULL, S_IRUGO); drivers/ntb/ntb_transport.c:68:module_param(max_num_clients, byte, 0644); drivers/scsi/lpfc/lpfc_attr.c:4207:module_param(lpfc_prot_guard, byte, S_IRUGO); drivers/usb/atm/speedtch.c:117:module_param(ModemMode, byte, S_IRUGO | S_IWUSR); drivers/usb/atm/speedtch.c:121:module_param_array(ModemOption, byte, &num_ModemOption, S_IRUGO); drivers/usb/gadget/g_ffs.c:95:module_param_named(bDeviceClass, gfs_dev_desc.bDeviceClass, byte, 0644); drivers/usb/gadget/g_ffs.c:97:module_param_named(bDeviceSubClass, gfs_dev_desc.bDeviceSubClass, byte, 0644); drivers/usb/gadget/g_ffs.c:99:module_param_named(bDeviceProtocol, gfs_dev_desc.bDeviceProtocol, byte, 0644); I have CC'd all the authors, to see if changing the results of reading /sys/module/<modname>/parameters/<xxx> from a literal char to a number will harm them. Thanks, Rusty. > --- > kernel/params.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/params.c b/kernel/params.c > index 440e65d..59f7ac7 100644 > --- a/kernel/params.c > +++ b/kernel/params.c > @@ -252,7 +252,7 @@ int parse_args(const char *doing, > EXPORT_SYMBOL(param_ops_##name) > > > -STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, strict_strtoul); > +STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", unsigned long, strict_strtoul); > STANDARD_PARAM_DEF(short, short, "%hi", long, strict_strtol); > STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, strict_strtoul); > STANDARD_PARAM_DEF(int, int, "%i", long, strict_strtol); > -- > 1.8.3.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] module: fix sprintf format specifier in param_get_byte() 2013-08-07 6:43 ` Rusty Russell @ 2013-08-07 11:31 ` Michal Nazarewicz 2013-08-07 16:45 ` Jon Mason 1 sibling, 0 replies; 5+ messages in thread From: Michal Nazarewicz @ 2013-08-07 11:31 UTC (permalink / raw) To: Rusty Russell, Christoph Jaeger Cc: linux-kernel, Dmitry Tarnyagin, Jon Mason, tech.support, David Woodhouse [-- Attachment #1: Type: text/plain, Size: 2207 bytes --] On Wed, Aug 07 2013, Rusty Russell wrote: > Christoph Jaeger <christophjaeger@linux.com> writes: >> In param_get_byte(), to which the macro STANDARD_PARAM_DEF(byte, ...) expands, >> "%c" is used to print an unsigned char. So it gets printed as a character what >> is not intended here. Use "%hhu" instead. >> >> Signed-off-by: Christoph Jaeger <christophjaeger@linux.com> Acked-by: Michal Nazarewicz <mina86@mina86.com> for g_ffs.c: > drivers/usb/gadget/g_ffs.c:95:module_param_named(bDeviceClass, gfs_dev_desc.bDeviceClass, byte, 0644); > drivers/usb/gadget/g_ffs.c:97:module_param_named(bDeviceSubClass, gfs_dev_desc.bDeviceSubClass, byte, 0644); > drivers/usb/gadget/g_ffs.c:99:module_param_named(bDeviceProtocol, gfs_dev_desc.bDeviceProtocol, byte, 0644); I don't think it breaks anything for g_ffs since those properties are most likely write-only and I don't expect many people reading them. >> --- >> kernel/params.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/params.c b/kernel/params.c >> index 440e65d..59f7ac7 100644 >> --- a/kernel/params.c >> +++ b/kernel/params.c >> @@ -252,7 +252,7 @@ int parse_args(const char *doing, >> EXPORT_SYMBOL(param_ops_##name) >> >> >> -STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, strict_strtoul); >> +STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", unsigned long, strict_strtoul); >> STANDARD_PARAM_DEF(short, short, "%hi", long, strict_strtol); >> STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, strict_strtoul); Actually, are those “h” specifiers even necessary? I'm fairly certain that “%u” for byte, “%i” for short, and “%u” for ushort would work just fine since the argument gets promoted to (unsigned) int anyway and indeed vsnprintf reads an int for all those types. >> STANDARD_PARAM_DEF(int, int, "%i", long, strict_strtol); -- Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz (o o) ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo-- [-- Attachment #2.1: Type: text/plain, Size: 0 bytes --] [-- Attachment #2.2: signature.asc --] [-- Type: application/pgp-signature, Size: 835 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] module: fix sprintf format specifier in param_get_byte() 2013-08-07 6:43 ` Rusty Russell 2013-08-07 11:31 ` Michal Nazarewicz @ 2013-08-07 16:45 ` Jon Mason 1 sibling, 0 replies; 5+ messages in thread From: Jon Mason @ 2013-08-07 16:45 UTC (permalink / raw) To: Rusty Russell Cc: Christoph Jaeger, linux-kernel, Dmitry Tarnyagin, tech.support, David Woodhouse, Michal Nazarewicz On Wed, Aug 07, 2013 at 04:13:57PM +0930, Rusty Russell wrote: > Christoph Jaeger <christophjaeger@linux.com> writes: > > In param_get_byte(), to which the macro STANDARD_PARAM_DEF(byte, ...) expands, > > "%c" is used to print an unsigned char. So it gets printed as a character what > > is not intended here. Use "%hhu" instead. > > > > Signed-off-by: Christoph Jaeger <christophjaeger@linux.com> > > Nice patch. Unfortunately, there are several users of this already: > > drivers/net/wireless/cw1200/main.c:50:module_param_array_named(macaddr, cw1200_mac_template, byte, NULL, S_IRUGO); > drivers/ntb/ntb_transport.c:68:module_param(max_num_clients, byte, 0644); This shouldn't affect NTB negatively. Acked-by: Jon Mason <jon.mason@intel.com> > drivers/scsi/lpfc/lpfc_attr.c:4207:module_param(lpfc_prot_guard, byte, S_IRUGO); > drivers/usb/atm/speedtch.c:117:module_param(ModemMode, byte, S_IRUGO | S_IWUSR); > drivers/usb/atm/speedtch.c:121:module_param_array(ModemOption, byte, &num_ModemOption, S_IRUGO); > drivers/usb/gadget/g_ffs.c:95:module_param_named(bDeviceClass, gfs_dev_desc.bDeviceClass, byte, 0644); > drivers/usb/gadget/g_ffs.c:97:module_param_named(bDeviceSubClass, gfs_dev_desc.bDeviceSubClass, byte, 0644); > drivers/usb/gadget/g_ffs.c:99:module_param_named(bDeviceProtocol, gfs_dev_desc.bDeviceProtocol, byte, 0644); > > I have CC'd all the authors, to see if changing the results of reading > /sys/module/<modname>/parameters/<xxx> from a literal char to a number > will harm them. > > Thanks, > Rusty. > > > --- > > kernel/params.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/params.c b/kernel/params.c > > index 440e65d..59f7ac7 100644 > > --- a/kernel/params.c > > +++ b/kernel/params.c > > @@ -252,7 +252,7 @@ int parse_args(const char *doing, > > EXPORT_SYMBOL(param_ops_##name) > > > > > > -STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, strict_strtoul); > > +STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", unsigned long, strict_strtoul); > > STANDARD_PARAM_DEF(short, short, "%hi", long, strict_strtol); > > STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, strict_strtoul); > > STANDARD_PARAM_DEF(int, int, "%i", long, strict_strtol); > > -- > > 1.8.3.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-08-07 16:46 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-08-06 21:12 [PATCH 1/1] module: fix sprintf format specifier in param_get_byte() Christoph Jaeger 2013-08-06 23:39 ` Joe Perches 2013-08-07 6:43 ` Rusty Russell 2013-08-07 11:31 ` Michal Nazarewicz 2013-08-07 16:45 ` Jon Mason
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.