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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.09.18 at 15:01, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/09/18 08:41, Jan Beulich wrote:
>>>>> On 06.09.18 at 14:08, <andrew.cooper3@xxxxxxxxxx> 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.

Well, I can accept the single suffix char as a good and useful extension.
I don't, however, think that making the suffixes arbitrarily long is too
good an idea.

>> 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. 

I'd drop the "very very", but then I agree. Yet we shouldn't slavishly
follow what they do, when it's questionable whether the direction is
indeed right. Hence my response here.

> 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.

Yeah, I started to be concerned of this happening after I had sent
the reply. Given this I guess we have no (good) choice besides going
the suffix route.

Jan



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