Re: [PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[Konrad Gräfe]

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@xxxxxxxxxxxxxxx
>
> Why cc stable?

I believe the second patch matches the criteria but it uses this one.

> > Signed-off-by: Konrad Gräfe <k.graefe@xxxxxxxxxxx>
> > ---
> > 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