Re: [PATCH v2] bitmap: remove explicit newline handling using scnprintf format string

[Sudeep Holla]

On 29/04/15 23:48, Andrew Morton wrote:
> On Tue, 28 Apr 2015 16:36:41 +0100 Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
>
> > bitmap_print_to_pagebuf uses scnprintf to copy the cpumask/list to page
> > buffer. It handles the newline and trailing null character explicitly.
>
> bitmap_print_to_pagebuf() is horrid :(  That automagic "assume caller
> passed us an offset into a PAGE_SIZE area".

Agreed.

> > It's unnecessary and also partially duplicated as scnprintf already adds
> > trailing null character. The newline can be passed through format string
> > to scnprintf. This patch does that simplification.
> >
> > However theoritically there's one behavior difference: when the buffer
> > is too small, the original code would still output '\n' at the end while
> > the new code(with this patch) would just continue to print the formatted
> > string. Since this function is dealing with only page buffers, it's
> > highly unlikely to hit that corner case.
> >
> > This patch will help in auditing the users of bitmap_print_to_pagebuf
> > to verify that the buffer passed is large enough and get rid of it
> > completely by replacing them with direct scnprintf()
>
> Yes, bitmap_print_to_pagebuf() should be eliminated.  Make the callers
> keep track of how much room they have in their buffer.

Yes that's what Tejun also mentioned in earlier mail. This patch is just
to audit all the callers and check if something breaks before we
replace this with scnprintf() at the call sites.

> > --- a/lib/bitmap.c
> > +++ b/lib/bitmap.c
> > @@ -462,19 +462,18 @@ EXPORT_SYMBOL(bitmap_parse_user);
> >    * Output format is a comma-separated list of decimal numbers and
> >    * ranges if list is specified or hex digits grouped into comma-separated
> >    * sets of 8 digits/set. Returns the number of characters written to buf.
> > + * It is assumed that the buffer passed is atleast PAGE_SIZE and easily
> > + * accommodate nmaskbits.
>
> Well kind-of.
>
> How does this look?

Looks good to me.

Regards,
Sudeep

> --- a/lib/bitmap.c~bitmap-remove-explicit-newline-handling-using-scnprintf-format-string-fix
> +++ a/lib/bitmap.c
> @@ -462,8 +462,10 @@ EXPORT_SYMBOL(bitmap_parse_user);
>    * Output format is a comma-separated list of decimal numbers and
>    * ranges if list is specified or hex digits grouped into comma-separated
>    * sets of 8 digits/set. Returns the number of characters written to buf.
> - * It is assumed that the buffer passed is atleast PAGE_SIZE and easily
> - * accommodate nmaskbits.
> + *
> + * It is assumed that @buf is a pointer into a PAGE_SIZE area and that
> + * sufficient storage remains at @buf to accommodate the
> + * bitmap_print_to_pagebuf() output.
>    */
>   int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
>   			    int nmaskbits)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/