Re: [PATCH 1/6] xen/vsprintf: Introduce %*pb[l] for printing bitmaps

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/09/18 08:41, Jan Beulich wrote:
>>>> On 06.09.18 at 14:08, <andrew.cooper3@citrix.com> wrote:
>> The format identifier is consistent with Linux.  The code is adapted from
>> bitmap_scn{,list}printf() but cleaned up.
> Irrespective of this I'm somewhat worried by ...
>
>> --- a/docs/misc/printk-formats.txt
>> +++ b/docs/misc/printk-formats.txt
>> @@ -13,6 +13,14 @@ Raw buffer as hex string:
>>         Up to 64 characters.  Buffer length expected via the field_width
>>         paramter. i.e. printk("%*ph", 8, buffer);
>>  
>> +Bitmaps (e.g. cpumask/nodemask):
>> +
>> +       %*pb    4321
>> +       %*pbl   0,5,8-9,14
>> +
>> +       Print a bitmap as either a hex string, or a range list.  Bitmap length
>> +       (in bits) expected via the field_width parameter.
> ... the l suffix here. It's not very likely that someone might mean to
> follow %pb by l, but it's syntactically ambiguous.

I don't see anything ambiguous here.  The l is for list, not for long,
and trailing modifiers are consistent with all the other %p infrastructure.

> Since the 'l' qualifier
> is so far meaningless for %p, why can't we use that instead, making
> usages look like %*lpb?

First and foremost, diverging from Linux's well-documented and well-used
API not something we should do without a very very good reason. 
Irrespective of whether you think it is ambiguous or not, I don't view
this as a good enough (potential) issue to diverge.

Furthermore, (and more likely to sway your opinion), N1570 indicates
that the 'l' length modifier is only applicable for the diouxXcs
conversion specifiers, and both Clang and GCC enforce this with -Wformat.

andrewcoop@andrewcoop:/local/xen.git/xen$ clang-6.0 -Wall -Werror -Wextra foo.c -o foo.o
foo.c:7:22: error: length modifier 'l' results in undefined behavior or no effect with 'p' conversion specifier [-Werror,-Wformat]
    printf("Testing %lpd\n", ptr);
                    ~^~
1 error generated.

andrewcoop@andrewcoop:/local/xen.git/xen$ gcc -Wall -Werror -Wextra foo.c -o foo.o
foo.c: In function ‘bar’:
foo.c:7:5: error: use of ‘l’ length modifier with ‘p’ type character [-Werror=format=]
     printf("Testing %lpd\n", ptr);
     ^
cc1: all warnings being treated as errors



>
>> --- a/xen/common/vsprintf.c
>> +++ b/xen/common/vsprintf.c
>> @@ -264,6 +264,88 @@ static char *string(char *str, char *end, const char 
>> *s,
>>      return str;
>>  }
>>  
>> +/* Print a bitmap as '0-3,6-15' */
>> +static char *print_bitmap_list(char *str, char *end,
>> +                               const unsigned long *bitmap, int nr_bits)
>> +{
>> +    /* current bit is 'cur', most recently seen range is [rbot, rtop] */
>> +    int cur, rbot, rtop;
> Including the nr_bits parameter - which of these really have to be
> plain (i.e. signed) int?

Hmm - overall, the bitmap API is a mix and match of signed-ness, both
for nr_bits, and the return value bit positions.

I think these probably can switch, while..

>
>> +/* Print a bitmap as a comma separated hex string. */
>> +static char *print_bitmap_string(char *str, char *end,
>> +                                 const unsigned long *bitmap, int nr_bits)
>> +{
>> +    const unsigned int CHUNKSZ = 32;
>> +    unsigned int chunksz;
>> +    int i;
> Same question here, despite ...
>
>> +    bool first = true;
>> +
>> +    chunksz = nr_bits & (CHUNKSZ - 1);
>> +    if ( chunksz == 0 )
>> +        chunksz = CHUNKSZ;
>> +
>> +    /*
>> +     * First iteration copes with the trailing partial word if nr_bits isn't a
>> +     * round multiple of CHUNKSZ.  All subsequent iterations work on a
>> +     * complete CHUNKSZ block.
>> +     */
>> +    for ( i = ROUNDUP(nr_bits, CHUNKSZ) - CHUNKSZ; i >= 0; i -= CHUNKSZ )
> ... this, which obviously would need adjustment if changed
> (and where hence it is at least worthwhile to consider leaving
> it the way it is).

... this should stay as it is because its by far the cleanest way of
expressing the logic.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel