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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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. Since the 'l' qualifier
is so far meaningless for %p, why can't we use that instead, making
usages look like %*lpb?

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

> +/* 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).


> @@ -273,6 +355,21 @@ static char *pointer(char *str, char *end, const char 
> **fmt_ptr,
>      /* Custom %p suffixes. See XEN_ROOT/docs/misc/printk-formats.txt */
>      switch ( fmt[1] )
>      {
> +    case 'b': /* Bitmap as hex, or list */
> +        ++*fmt_ptr;
> +
> +        if ( field_width < 0 )
> +            return str;
> +
> +        if ( fmt[2] == 'l' )
> +        {
> +            ++*fmt_ptr;

With the suffixing approach an anomaly will result here when the
"field_width < 0" path is taken above, leaving *fmt_ptr point at
the 'l'.

Jan



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