| Message ID | 20230427115120.241954-1-k.graefe@gateware.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] vsprintf: Add %p[mM]U for uppercase MAC address | expand |
On Thu, Apr 27, 2023 at 01:51:19PM +0200, Konrad Gräfe wrote: > The CDC-ECM specification requires an USB gadget to send the host MAC > address as uppercase hex string. This change adds the appropriate > modifier. > > Cc: stable@vger.kernel.org > Signed-off-by: Konrad Gräfe <k.graefe@gateware.de> > --- > Added in v3 > > lib/vsprintf.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index be71a03c936a..8aee1caabd9e 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1269,9 +1269,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr, > { > char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")]; > char *p = mac_addr; > - int i; > + int i, pos; > char separator; > bool reversed = false; > + bool uppercase = false; > > if (check_pointer(&buf, end, addr, spec)) > return buf; > @@ -1281,6 +1282,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr, > separator = '-'; > break; > > + case 'U': > + uppercase = true; > + break; No documentation update as well? thanks, greg k-h
On 27/04/2023 13.51, Konrad Gräfe wrote: > The CDC-ECM specification requires an USB gadget to send the host MAC > address as uppercase hex string. This change adds the appropriate > modifier. > > Cc: stable@vger.kernel.org Why cc stable? > Signed-off-by: Konrad Gräfe <k.graefe@gateware.de> > --- > Added in v3 > > lib/vsprintf.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) The diffstat here, or for some other patch in the same series, definitely ought to mention lib/test_printf.c. > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index be71a03c936a..8aee1caabd9e 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1269,9 +1269,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr, > { > char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")]; > char *p = mac_addr; > - int i; > + int i, pos; > char separator; > bool reversed = false; > + bool uppercase = false; > > if (check_pointer(&buf, end, addr, spec)) > return buf; > @@ -1281,6 +1282,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr, > separator = '-'; > break; > > + case 'U': > + uppercase = true; > + break; > + > case 'R': > reversed = true; > fallthrough; This seems broken, and I'm surprised the compiler doesn't warn about separator possibly being uninitialized further down. I'm also surprised your testing hasn't caught this. For reference, the full switch statement is currently switch (fmt[1]) { case 'F': separator = '-'; break; case 'R': reversed = true; fallthrough; default: separator = ':'; break; } > @@ -1292,9 +1297,14 @@ char *mac_address_string(char *buf, char *end, u8 *addr, > > for (i = 0; i < 6; i++) { > if (reversed) > - p = hex_byte_pack(p, addr[5 - i]); > + pos = 5 - i; > + else > + pos = i; > + > + if (uppercase) > + p = hex_byte_pack_upper(p, addr[pos]); > else > - p = hex_byte_pack(p, addr[i]); > + p = hex_byte_pack(p, addr[pos]); I think this becomes quite hard to follow. We have string_upper() in linux/string_helpers.h, so I'd rather just leave this loop alone and do if (uppercase) string_upper(mac_addr, mac_addr); after the nul-termination. Rasmus
On 27.04.23 14:35, Rasmus Villemoes wrote: > On 27/04/2023 13.51, Konrad Gräfe wrote: >> The CDC-ECM specification requires an USB gadget to send the host MAC >> address as uppercase hex string. This change adds the appropriate >> modifier. >> >> Cc: stable@vger.kernel.org > > Why cc stable? I believe the second patch matches the criteria but it uses this one. > >> Signed-off-by: Konrad Gräfe <k.graefe@gateware.de> >> --- >> Added in v3 >> >> lib/vsprintf.c | 18 +++++++++++++++--- >> 1 file changed, 15 insertions(+), 3 deletions(-) > > The diffstat here, or for some other patch in the same series, > definitely ought to mention lib/test_printf.c. > >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c >> index be71a03c936a..8aee1caabd9e 100644 >> --- a/lib/vsprintf.c >> +++ b/lib/vsprintf.c >> @@ -1269,9 +1269,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr, >> { >> char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")]; >> char *p = mac_addr; >> - int i; >> + int i, pos; >> char separator; >> bool reversed = false; >> + bool uppercase = false; >> >> if (check_pointer(&buf, end, addr, spec)) >> return buf; >> @@ -1281,6 +1282,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr, >> separator = '-'; >> break; >> >> + case 'U': >> + uppercase = true; >> + break; >> + >> case 'R': >> reversed = true; >> fallthrough; > > This seems broken, and I'm surprised the compiler doesn't warn about > separator possibly being uninitialized further down. I'm also surprised > your testing hasn't caught this. For reference, the full switch > statement is currently > > switch (fmt[1]) { > case 'F': > separator = '-'; > break; > > case 'R': > reversed = true; > fallthrough; > > default: > separator = ':'; > break; > } > >> @@ -1292,9 +1297,14 @@ char *mac_address_string(char *buf, char *end, u8 *addr, >> >> for (i = 0; i < 6; i++) { >> if (reversed) >> - p = hex_byte_pack(p, addr[5 - i]); >> + pos = 5 - i; >> + else >> + pos = i; >> + >> + if (uppercase) >> + p = hex_byte_pack_upper(p, addr[pos]); >> else >> - p = hex_byte_pack(p, addr[i]); >> + p = hex_byte_pack(p, addr[pos]); > > I think this becomes quite hard to follow. We have string_upper() in > linux/string_helpers.h, so I'd rather just leave this loop alone and do > > if (uppercase) > string_upper(mac_addr, mac_addr); > > after the nul-termination. > > Rasmus >
Hello Rasmus, Konrad, *, On Thu, 27 Apr 2023 14:35:19 +0200, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > On 27/04/2023 13.51, Konrad Gräfe wrote: > > The CDC-ECM specification requires an USB gadget to send the host MAC > > address as uppercase hex string. This change adds the appropriate > > modifier. > > > > Cc: stable@vger.kernel.org > > Why cc stable? > > > Signed-off-by: Konrad Gräfe <k.graefe@gateware.de> > > --- > > Added in v3 > > > > lib/vsprintf.c | 18 +++++++++++++++--- > > 1 file changed, 15 insertions(+), 3 deletions(-) > > The diffstat here, or for some other patch in the same series, > definitely ought to mention lib/test_printf.c. > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > index be71a03c936a..8aee1caabd9e 100644 > > --- a/lib/vsprintf.c > > +++ b/lib/vsprintf.c > > @@ -1269,9 +1269,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr, > > { > > char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")]; > > char *p = mac_addr; > > - int i; > > + int i, pos; > > char separator; > > bool reversed = false; > > + bool uppercase = false; > > > > if (check_pointer(&buf, end, addr, spec)) > > return buf; > > @@ -1281,6 +1282,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr, > > separator = '-'; > > break; > > > > + case 'U': > > + uppercase = true; > > + break; > > + > > case 'R': > > reversed = true; > > fallthrough; > > This seems broken, and I'm surprised the compiler doesn't warn about > separator possibly being uninitialized further down. I'm also surprised > your testing hasn't caught this. For reference, the full switch > statement is currently Compiler (gcc) does not warn because of Makefile: 1038 # Enabled with W=2, disabled by default as noisy 1039 ifdef CONFIG_CC_IS_GCC 1040 KBUILD_CFLAGS += -Wno-maybe-uninitialized 1041 endif With this commented: lib/vsprintf.c: In function ‘mac_address_string’: lib/vsprintf.c:1310:30: warning: ‘separator’ may be used uninitialized [-Wmaybe-uninitialized] 1310 | *p++ = separator; | ~~~~~^~~~~~~~~~~ lib/vsprintf.c:1273:14: note: ‘separator’ was declared here 1273 | char separator; | ^~~~~~~~~ Regards, Peter > > switch (fmt[1]) { > case 'F': > separator = '-'; > break; > > case 'R': > reversed = true; > fallthrough; > > default: > separator = ':'; > break; > } > > > @@ -1292,9 +1297,14 @@ char *mac_address_string(char *buf, char *end, u8 *addr, > > > > for (i = 0; i < 6; i++) { > > if (reversed) > > - p = hex_byte_pack(p, addr[5 - i]); > > + pos = 5 - i; > > + else > > + pos = i; > > + > > + if (uppercase) > > + p = hex_byte_pack_upper(p, addr[pos]); > > else > > - p = hex_byte_pack(p, addr[i]); > > + p = hex_byte_pack(p, addr[pos]); > > I think this becomes quite hard to follow. We have string_upper() in > linux/string_helpers.h, so I'd rather just leave this loop alone and do > > if (uppercase) > string_upper(mac_addr, mac_addr); > > after the nul-termination. > > Rasmus >
On 27/04/2023 13.51, Konrad Gräfe wrote: > The CDC-ECM specification requires an USB gadget to send the host MAC > address as uppercase hex string. This change adds the appropriate > modifier. Thinking more about it, I'm not sure this is appropriate, not for a single user like this. vsprintf() should not and cannot satisfy all possible string formatting requirements for the whole kernel. The %pX extensions are convenient for use with printk() and friends where one needs what in other languages would be "string interpolation" (because then the caller doesn't need to deal with temporary stack buffers and pass them as %s arguments), but for single items like this, snprintf() is not necessarily the right tool for the job. In this case, the caller can just as well call string_upper() on the result, or not use sprintf() at all and do a tiny loop with hex_byte_pack_upper(). Rasmus
On Fri, Apr 28, 2023 at 08:56:59AM +0200, Rasmus Villemoes wrote: > On 27/04/2023 13.51, Konrad Gräfe wrote: > > The CDC-ECM specification requires an USB gadget to send the host MAC > > address as uppercase hex string. This change adds the appropriate > > modifier. > > Thinking more about it, I'm not sure this is appropriate, not for a > single user like this. vsprintf() should not and cannot satisfy all > possible string formatting requirements for the whole kernel. The %pX > extensions are convenient for use with printk() and friends where one > needs what in other languages would be "string interpolation" (because > then the caller doesn't need to deal with temporary stack buffers and > pass them as %s arguments), but for single items like this, snprintf() > is not necessarily the right tool for the job. But sprintf() already creates mac address strings today, adding yet-another-modifier makes it so that we don't have to hand-roll this type of logic in the individual drivers that require it. thanks, greg k-h
From: Rasmus Villemoes > Sent: 28 April 2023 07:57 > > On 27/04/2023 13.51, Konrad Gräfe wrote: > > The CDC-ECM specification requires an USB gadget to send the host MAC > > address as uppercase hex string. This change adds the appropriate > > modifier. > > Thinking more about it, I'm not sure this is appropriate, not for a > single user like this. vsprintf() should not and cannot satisfy all > possible string formatting requirements for the whole kernel. The %pX > extensions are convenient for use with printk() and friends where one > needs what in other languages would be "string interpolation" (because > then the caller doesn't need to deal with temporary stack buffers and > pass them as %s arguments), but for single items like this, snprintf() > is not necessarily the right tool for the job. > > In this case, the caller can just as well call string_upper() on the > result, or not use sprintf() at all and do a tiny loop with > hex_byte_pack_upper(). Or snprintf with "%02X:%02X:%02X:%02X:%02X:%02X". David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri 2023-04-28 08:56:59, Rasmus Villemoes wrote: > On 27/04/2023 13.51, Konrad Gräfe wrote: > > The CDC-ECM specification requires an USB gadget to send the host MAC > > address as uppercase hex string. This change adds the appropriate > > modifier. > > Thinking more about it, I'm not sure this is appropriate, not for a > single user like this. vsprintf() should not and cannot satisfy all > possible string formatting requirements for the whole kernel. The %pX > extensions are convenient for use with printk() and friends where one > needs what in other languages would be "string interpolation" (because > then the caller doesn't need to deal with temporary stack buffers and > pass them as %s arguments), but for single items like this, snprintf() > is not necessarily the right tool for the job. > > In this case, the caller can just as well call string_upper() on the > result I tend to agree with Rasmus. string_upper() is a super-easy solution. One user does not look worth adding all the churn into vsprintf(). Best Regards, Petr
On Fri, Apr 28, 2023 at 07:46:14AM +0000, David Laight wrote: > From: Rasmus Villemoes > > Sent: 28 April 2023 07:57 > > On 27/04/2023 13.51, Konrad Gräfe wrote: > > > The CDC-ECM specification requires an USB gadget to send the host MAC > > > address as uppercase hex string. This change adds the appropriate > > > modifier. > > > > Thinking more about it, I'm not sure this is appropriate, not for a > > single user like this. vsprintf() should not and cannot satisfy all > > possible string formatting requirements for the whole kernel. The %pX > > extensions are convenient for use with printk() and friends where one > > needs what in other languages would be "string interpolation" (because > > then the caller doesn't need to deal with temporary stack buffers and > > pass them as %s arguments), but for single items like this, snprintf() > > is not necessarily the right tool for the job. > > > > In this case, the caller can just as well call string_upper() on the > > result, or not use sprintf() at all and do a tiny loop with > > hex_byte_pack_upper(). > > Or snprintf with "%02X:%02X:%02X:%02X:%02X:%02X". Of course this is a step back. Why? Have you read actually what we have in %p extensions already? Also, what about stack? Entire %pm/M exists due to reversed order. Otherwise it's an alias to %6phD or alike.
On 02.05.23 14:23, Petr Mladek wrote: > On Fri 2023-04-28 08:56:59, Rasmus Villemoes wrote: >> On 27/04/2023 13.51, Konrad Gräfe wrote: >>> The CDC-ECM specification requires an USB gadget to send the host MAC >>> address as uppercase hex string. This change adds the appropriate >>> modifier. >> >> Thinking more about it, I'm not sure this is appropriate, not for a >> single user like this. vsprintf() should not and cannot satisfy all >> possible string formatting requirements for the whole kernel. The %pX >> extensions are convenient for use with printk() and friends where one >> needs what in other languages would be "string interpolation" (because >> then the caller doesn't need to deal with temporary stack buffers and >> pass them as %s arguments), but for single items like this, snprintf() >> is not necessarily the right tool for the job. >> >> In this case, the caller can just as well call string_upper() on the >> result > > I tend to agree with Rasmus. string_upper() is a super-easy solution. > One user does not look worth adding all the churn into vsprintf(). > > Best Regards, > Petr I do agree as well. That would basically be v1 [1] without the hand-crafted string_upper(). (I didn't know the function.) Regards, Konrad [1]: https://lore.kernel.org/linux-usb/94afd6e0-7300-e8f4-d52e-c21acec04f5b@gateware.de/
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index be71a03c936a..8aee1caabd9e 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1269,9 +1269,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr, { char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")]; char *p = mac_addr; - int i; + int i, pos; char separator; bool reversed = false; + bool uppercase = false; if (check_pointer(&buf, end, addr, spec)) return buf; @@ -1281,6 +1282,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr, separator = '-'; break; + case 'U': + uppercase = true; + break; + case 'R': reversed = true; fallthrough; @@ -1292,9 +1297,14 @@ char *mac_address_string(char *buf, char *end, u8 *addr, for (i = 0; i < 6; i++) { if (reversed) - p = hex_byte_pack(p, addr[5 - i]); + pos = 5 - i; + else + pos = i; + + if (uppercase) + p = hex_byte_pack_upper(p, addr[pos]); else - p = hex_byte_pack(p, addr[i]); + p = hex_byte_pack(p, addr[pos]); if (fmt[0] == 'M' && i != 5) *p++ = separator; @@ -2279,6 +2289,7 @@ char *rust_fmt_argument(char *buf, char *end, void *ptr); * - 'm' For a 6-byte MAC address, it prints the hex address without colons * - 'MF' For a 6-byte MAC FDDI address, it prints the address * with a dash-separated hex notation + * - '[mM]U' For a 6-byte MAC address in uppercase hex * - '[mM]R' For a 6-byte MAC address, Reverse order (Bluetooth) * - 'I' [46] for IPv4/IPv6 addresses printed in the usual way * IPv4 uses dot-separated decimal without leading 0's (1.2.3.4) @@ -2407,6 +2418,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, case 'M': /* Colon separated: 00:01:02:03:04:05 */ case 'm': /* Contiguous: 000102030405 */ /* [mM]F (FDDI) */ + /* [mM]U (Uppercase hex) */ /* [mM]R (Reverse order; Bluetooth) */ return mac_address_string(buf, end, ptr, spec, fmt); case 'I': /* Formatted IP supported
The CDC-ECM specification requires an USB gadget to send the host MAC address as uppercase hex string. This change adds the appropriate modifier. Cc: stable@vger.kernel.org Signed-off-by: Konrad Gräfe <k.graefe@gateware.de> --- Added in v3 lib/vsprintf.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)