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

[Andrew Morton]

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

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

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

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