lkml.org 
[lkml]   [2004]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] bitmap parsing/printing routines, version 4
    Joe Korty wrote:
    > This release of the bitmap parser/printer continues the cycle of making
    > ever-smaller changes to the base code.
    >
    > The most significant change this time around is to be sure every
    > character in the string is scanned for correctness. In the previous
    > version, the scan would stop on the first whitespace character following
    > a non-whitespace character. There was no determination made if that was
    > the start of either embedded or trailing whitspace -- it was assumed to
    > be trailing.
    >
    > I gradually came to feel that examining the whole string was important, as
    > the parser does not return a pointer to where it stopped as the string(3)
    > family does, so it is not possible for the caller to easily determine
    > if the condition stopping the scan was reasonable or not.
    >
    > ChangeLog:
    > o remove trailing whitespace early termination.
    > o add leading / embedded / trailing whitespace tests.
    > o 'extern' not needed on prototypes (Bill Irwin)
    > o made terse variable names readable.
    > o continued the reduction of the algorithm to simpler forms.
    >
    > This release concludes my contributions to this patch, other than changes
    > driven by user requests and experience.
    >
    > Against 2.6.1-mm4.
    >
    > Joe

    Joe,
    I've attatched a small patch with some *small* changes, and the
    addition of a whole lotta comments. I'd like to see what you think.

    Changes:
    1) Added a missing '"' in the comment for the bitmap_parse function
    2) Renamed 'oc' to 'old_c' for readability
    3) Remove "totaldigits == 0" check at the end of bitmap_parse. I
    believe this check is redundant. The only way that totaldigits could be
    0 at the end of the function is if ndigits is also 0 (because they're
    both incremented at the same time), and this condition is already
    checked for at the end of each chunk parsed. Is this correct?

    Additions:
    4) A whole bunch of comments. Are these all correct?

    None of the things in my patch (with the possible exception of #3)
    change the functionality of the code, which looks great.

    Andrew, I agree with Paul's "thumbs-up" of Joe's patch. My patch is
    solely meant to increase the readability of the bitmap_parse function.

    Cheers!

    -Matt
    --- linux-2.6.1-joe_korty-bitmap/lib/bitmap.c.orig Mon Jan 19 11:45:32 2004
    +++ linux-2.6.1-joe_korty-bitmap/lib/bitmap.c Mon Jan 19 13:11:24 2004
    @@ -209,13 +209,13 @@ EXPORT_SYMBOL(bitmap_snprintf);
    * bits of the resultant bitmask. No chunk may specify a value larger
    * than 32 bits (-EOVERFLOW), and if a chunk specifies a smaller value
    * then leading 0-bits are prepended. -EINVAL is returned for illegal
    - * characters and for grouping errors such as "1,,5", ,44", "," and "".
    + * characters and for grouping errors such as "1,,5", ",44", "," and "".
    * Leading and trailing whitespace accepted, but not embedded whitespace.
    */
    int bitmap_parse(const char __user *ubuf, unsigned int ubuflen,
    unsigned long *maskp, int nmaskbits)
    {
    - int i, c, oc, ndigits, totaldigits, nchunks, nbits;
    + int i, c, old_c, totaldigits, ndigits, nchunks, nbits;
    u32 chunk;

    bitmap_clear(maskp, nmaskbits);
    @@ -223,40 +223,73 @@ int bitmap_parse(const char __user *ubuf
    nchunks = nbits = totaldigits = c = 0;
    do {
    chunk = ndigits = 0;
    +
    + /* Get the next chunk of the bitmap */
    while (ubuflen) {
    - oc = c;
    + /* Remember the last char & get the next char */
    + old_c = c;
    if (get_user(c, ubuf++))
    return -EFAULT;
    ubuflen--;
    +
    + /* Ignore spaces */
    if (isspace(c))
    continue;
    - if (totaldigits && c && isspace(oc))
    +
    + /*
    + * If the last character was a space and the current
    + * character isn't '\0' we've got embedded whitespace.
    + * This is a no-no, so throw an error.
    + */
    + if (totaldigits && c && isspace(old_c))
    return -EINVAL;
    +
    + /* A '\0' or a ',' signal the end of the chunk */
    if (!c || c == ',')
    break;
    +
    + /* A non-hexdigit is also a no-no, so throw an error */
    if (!isxdigit(c))
    return -EINVAL;
    +
    + /*
    + * Make sure there are at least 4 free bits in 'chunk'.
    + * If not, this hexdigit will overflow 'chunk', so
    + * throw an error.
    + */
    if (chunk & ~((1UL << (CHUNKSZ - 4)) - 1))
    return -EOVERFLOW;
    +
    + /*
    + * Add this expanded hexdigit to 'chunk' and increment
    + * both the current chunk & total digit counts.
    + */
    chunk = (chunk << 4) | unhex(c);
    ndigits++; totaldigits++;
    }
    + /* Empty chunks are another no-no. Throw an error. */
    if (ndigits == 0)
    return -EINVAL;
    +
    + /* If the first chunk is all 0's, just move to the next one */
    if (nchunks == 0 && chunk == 0)
    continue;
    +
    + /* Shift the bitmap right to make room for this chunk */
    bitmap_shift_right(maskp, maskp, CHUNKSZ, nmaskbits);
    +
    + /* Copy the chunk into the bitmap, and increment chunk count */
    for (i = 0; i < CHUNKSZ; i++)
    if (chunk & (1 << i))
    set_bit(i, maskp);
    nchunks++;
    +
    + /* Increment the bit count & make sure we didn't overflow */
    nbits += (nchunks == 1) ? nbits_to_hold_value(chunk) : CHUNKSZ;
    if (nbits > nmaskbits)
    return -EOVERFLOW;
    } while (ubuflen && c == ',');

    - if (totaldigits == 0)
    - return -EINVAL;
    return 0;
    }
    EXPORT_SYMBOL(bitmap_parse);
    \
     
     \ /
      Last update: 2005-03-22 14:00    [W:0.038 / U:0.072 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site