Re: [PATCH 1/1] module: fix sprintf format specifier in param_get_byte()
On Wed, Aug 07, 2013 at 04:13:57PM +0930, Rusty Russell wrote: > Christoph Jaeger 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 > > 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 > 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, > _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//parameters/ 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] module: fix sprintf format specifier in param_get_byte()
On Wed, Aug 07 2013, Rusty Russell wrote: > Christoph Jaeger 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 Acked-by: Michal Nazarewicz 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 +--ooO--(_)--Ooo-- signature.asc Description: PGP signature
Re: [PATCH 1/1] module: fix sprintf format specifier in param_get_byte()
Christoph Jaeger 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 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, _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//parameters/ 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] module: fix sprintf format specifier in param_get_byte()
Christoph Jaeger christophjae...@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 christophjae...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] module: fix sprintf format specifier in param_get_byte()
On Wed, Aug 07 2013, Rusty Russell wrote: Christoph Jaeger christophjae...@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 christophjae...@linux.com Acked-by: Michal Nazarewicz min...@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: m...@google.com--ooO--(_)--Ooo-- signature.asc Description: PGP signature
Re: [PATCH 1/1] module: fix sprintf format specifier in param_get_byte()
On Wed, Aug 07, 2013 at 04:13:57PM +0930, Rusty Russell wrote: Christoph Jaeger christophjae...@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 christophjae...@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.ma...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] module: fix sprintf format specifier in param_get_byte()
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 > --- > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] module: fix sprintf format specifier in param_get_byte()
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 christophjae...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
