On Mon, Feb 10, 2014 at 05:09:12PM -0600, Mark Tinguely wrote:
> Setting the directory startoff, startblock, and blockcount
> fields are difficult on both big and little endian machines.
> The setting of extentflag was completely broken.
.....
> + char *in = ibuf;
> + char *out = obuf;
> + int bit;
> +
> + if (bitoff % NBBY || nbits % NBBY) {
> + for (bit=0; bit<nbits; bit++)
> + setbit(out, bit+bitoff, getbit(in, bit));
> + } else
> + memcpy(out+byteize(bitoff), in, byteize(nbits));
Still whitespace challenged ;)
> Index: b/db/write.c
> ===================================================================
> --- a/db/write.c
> +++ b/db/write.c
> @@ -439,55 +439,78 @@ convert_oct(
>
> #define NYBBLE(x) (isdigit(x)?(x-'0'):(tolower(x)-'a'+0xa))
>
> +/*
> + * convert_arg allow input in the following forms:
> + * A string ("ABTB") whose ASCII value is placed in an array in the order
> + * matching the input.
> + *
> + * An even number of hex numbers. If the length is greater than
> + * 64 bits, then the output is an array of bytes whose top nibble
> + * is the first hex digit in the input, the lower nibble is the second
> + * hex digit in the input. UUID entries are entered in this manner.
> + * If the length is 64 bits or less, then treat the hex input characters
> + * as a number used with setbitval().
> + *
> + * A decimal or hexadecimal integer to be used with setbitval().
> + * ---
> + * Numbers that will use setbitval() are in big endian format and
> + * are adjusted in the array so that the first input bit is to be
> + * be written to the first bit in the output.
> + */
> static char *
> convert_arg(
> - char *arg,
> - int bit_length)
> + char *arg,
> + int bit_length)
> {
> - int i;
> - static char *buf = NULL;
> - char *rbuf;
> - long long *value;
> - int alloc_size;
> - char *ostr;
> - int octval, ret;
> + int i;
> + int alloc_size;
> + int octval;
> + int offset;
> + int ret;
> + static char *buf = NULL;
> + char *endp;
> + char *rbuf;
> + char *ostr;
> + __u64 *value;
> + __u64 val = 0;
>
> if (bit_length <= 64)
> alloc_size = 8;
> else
> - alloc_size = (bit_length+7)/8;
> + alloc_size = (bit_length + 7)/8;
alloc_size = (bit_length + 7) / 8;
>
> buf = xrealloc(buf, alloc_size);
> memset(buf, 0, alloc_size);
> - value = (long long *)buf;
> + value = (__u64 *)buf;
> rbuf = buf;
>
> if (*arg == '\"') {
> - /* handle strings */
> + /* input a string and output ASCII array of characters */
>
> /* zap closing quote if there is one */
> - if ((ostr = strrchr(arg+1, '\"')) != NULL)
> + if ((ostr = strrchr(arg + 1, '\"')) != NULL)
> *ostr = '\0';
If you are touching these, the preferred form is
ostr = strrchr(arg + 1, '\"');
if (ostr)
*ostr = '\0';
>
> - ostr = arg+1;
> + ostr = arg + 1;
> for (i = 0; i < alloc_size; i++) {
> if (!*ostr)
> break;
>
> - /* do octal */
> + /* do octal conversion */
> if (*ostr == '\\') {
> - if (*(ostr+1) >= '0' || *(ostr+1) <= '7') {
> - ret = convert_oct(ostr+1, &octval);
> + if (*(ostr + 1) >= '0' || *(ostr + 1) <= '7') {
> + ret = convert_oct(ostr + 1, &octval);
> *rbuf++ = octval;
> - ostr += ret+1;
> + ostr += ret + 1;
> continue;
> }
> }
> *rbuf++ = *ostr++;
> }
> -
> return buf;
> - } else if (arg[0] == '#' || ((arg[0] != '-') && strchr(arg,'-'))) {
> + }
> +
> + if (arg[0] == '#' || ((arg[0] != '-') && strchr(arg,'-'))) {
> /*
> * handle hex blocks ie
> * #00112233445566778899aabbccddeeff
> @@ -495,49 +518,89 @@ convert_arg(
> * 1122334455667788-99aa-bbcc-ddee-ff00112233445566778899
> *
> * (but if it starts with "-" assume it's just an integer)
> + *
> + * Treat all requests 64 bits or smaller as numbers and
> + * requests larger than 64 bits as hex blocks arrays.
> */
> - int bytes=bit_length/8;
> + int bytes = bit_length / NBBY;
>
> /* skip leading hash */
> - if (*arg=='#') arg++;
> + if (*arg == '#') arg++;
>
> while (*arg && bytes--) {
> - /* skip hypens */
> - while (*arg=='-') arg++;
> + /* skip hypens */
> + while (*arg == '-') arg++;
while (*arg == '-')
arg++;
>
> - /* get first nybble */
> - if (!isxdigit((int)*arg)) return NULL;
> - *rbuf=NYBBLE((int)*arg)<<4;
> - arg++;
> -
> - /* skip more hyphens */
> - while (*arg=='-') arg++;
> -
> - /* get second nybble */
> - if (!isxdigit((int)*arg)) return NULL;
> - *rbuf++|=NYBBLE((int)*arg);
> - arg++;
> + /* get first nybble */
> + if (!isxdigit((int)*arg))
> + return NULL;
> + if (bit_length <= 64)
> + val = (val << 4) + NYBBLE((int)*arg);
> + else
> + *rbuf = NYBBLE((int)*arg) << 4;
> + arg++;
> +
> + /* skip more hyphens */
> + while (*arg == '-')
> + arg++;
> +
> + /* get second nybble */
> + if (!isxdigit((int)*arg))
> + return NULL;
> + if (bit_length <= 64)
> + val = (val << 4) + NYBBLE((int)*arg);
> + else
> + *rbuf++ |= NYBBLE((int)*arg);
> + arg++;
> }
> - if (bytes<0&&*arg) return NULL;
> - return buf;
> - } else {
> + if (bytes < 0 && *arg)
> + return NULL;
> +
> /*
> - * handle integers
> + * Values larger than 64 bits are array of hex digits that
> + * already in the desired ordering (example UUID).
> */
> - *value = strtoll(arg, NULL, 0);
> + if (bit_length > 64)
> + return buf;
I don't understand why you added this - how can we have input left
that we need to parse after the above loop? @bytes will always be <=
0 at this point in time, which means we have no space in the bit
field left to put values into....
> + } else {
> + /* handle decimal / hexadecimal integers */
>
> -#if __BYTE_ORDER == BIG_ENDIAN
> - /* hackery for big endian */
> - if (bit_length <= 8) {
> - rbuf += 7;
> - } else if (bit_length <= 16) {
> - rbuf += 6;
> - } else if (bit_length <= 32) {
> - rbuf += 4;
> - }
> -#endif
> - return rbuf;
> + val = strtoll(arg, &endp, 0);
> + /* return if not a clean number */
> + if (*endp != '\0')
> + return NULL;
> }
> +
> + /* Is this value valid for the bit length? */
> + if (val >> bit_length)
> + return NULL;
That's quite ambiguous - it's not obviously correct as it requires
close attention to determine that the code actually functions
correctly. i.e. I had to look very closely to determine ">>" or ">"
should have been used here. Can you rewrite it as:
/* Check if the value can fit into the supplied bitfield */
if ((val >> bit_length) > 0)
return NULL;
> + /*
> + * If the length of the field is not a multiple of a byte, push
> + * the bits up in the field, so the most signicant field bit is
> + * the most significant bit in the byte:
> + *
> + * before:
> + * val |----|----|----|----|----|--MM|mmmm|llll|
> + * after
> + * val |----|----|----|----|----|MMmm|mmll|ll00|
> + */
> + offset = bit_length % NBBY;
> + if (offset)
> + val <<= (NBBY - offset);
> +
> + /*
> + * convert to big endian and copy into the array
> + * rbuf |----|----|----|----|----|MMmm|mmll|ll00|
> + */
> + *value = cpu_to_be64(val);
> +
> + /*
> + * Align the array to point to the field in the array.
> + * rbuf = |MMmm|mmll|ll00|
> + */
> + offset = sizeof(__be64) - 1 - ((bit_length - 1)/ sizeof(__be64));
^^
whitespace
Other that that, the comments greatly improve this code. Thanks for
doing that.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
