On Thu, Feb 13, 2014 at 02:25:36PM -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.
>
> big endian test:
> xfs_db> write u.bmx[0].startblock 12
> u.bmx[0].startblock = 0
> xfs_db> write u.bmx[0].startblock 0xc0000
> u.bmx[0].startblock = 192
>
> little endian test:
> xfs_db> write u.bmx[0].startblock 12
> u.bmx[0].startblock = 211106232532992
> xfs_db> write u.bmx[0].startblock 0xc0000
> u.bmx[0].startblock = 3221225472
>
> Since the output fields and the lengths are not aligned to a byte,
> setbitval requires them to be entered in big endian and properly
> byte/nibble shifted. The extentflag output was aligned to a byte
> but was not shifted correctly.
>
> Convert the input to big endian on little endian machines and
> nibble/byte shift on all platforms so setbitval can set the bits
> correctly.
>
> Clean some whitespace while in the setbitbal() function.
>
> Signed-off-by: Mark Tinguely <tinguely@xxxxxxx>
> ---
> v3:
> Hex input is not a number.
> More ten year old white space cleanups.
>
> v2:
> Ignore extra characters in input.
> Fix hash input if still used as an integer input.
> It was broken on big endian, but someone may use this input in a
> little endian script.
> Add documentation.
> Did more clean up.
>
> db/bit.c | 84 ++++++++++++------------------
> db/write.c | 166 ++++++++++++++++++++++++++++++++++++++++---------------------
> 2 files changed, 143 insertions(+), 107 deletions(-)
>
> Index: b/db/bit.c
> ===================================================================
> --- a/db/bit.c
> +++ b/db/bit.c
> @@ -128,57 +128,41 @@ getbitval(
> return rval;
> }
>
> +/*
> + * The input data can be 8, 16, 32, and 64 sized numeric values
> + * aligned on a byte boundry, or odd sized numbers stored on odd
> + * aligned offset (for example the bmbt fields).
> + *
> + * The input data sent to this routine has been converted to big endian
> + * and has been adjusted in the array so that the first input bit is to
> + * be written in the first bit in the output.
> + *
> + * If the field length and the output buffer are byte aligned, then use
> + * memcpy from the input to the output, but if either entries are not byte
> + * aligned, then loop over the entire bit range reading the input value
> + * and set/clear the matching bit in the output.
> + *
> + * example when ibuf is not multiple of a byte in length:
> + *
> + * ibuf: | BBBBBBBB | bbbxxxxx |
> + * \\\\\\\\--\\\\
> + * obuf+bitoff: | xBBBBBBB | Bbbbxxxx |
> + *
> + */
> void
> setbitval(
> - void *obuf, /* buffer to write into */
> - int bitoff, /* bit offset of where to write */
> - int nbits, /* number of bits to write */
> - void *ibuf) /* source bits */
> + void *obuf, /* start of buffer to write into */
> + int bitoff, /* bit offset into the output buffer */
> + int nbits, /* number of bits to write */
> + void *ibuf) /* source bits */
> {
> - char *in = (char *)ibuf;
> - char *out = (char *)obuf;
> -
> - int bit;
> -
> -#if BYTE_ORDER == LITTLE_ENDIAN
> - int big = 0;
> -#else
> - int big = 1;
> -#endif
> -
> - /* only need to swap LE integers */
> - if (big || (nbits!=16 && nbits!=32 && nbits!=64) ) {
> - /* We don't have type info, so we can only assume
> - * that 2,4 & 8 byte values are integers. sigh.
> - */
> -
> - /* byte aligned ? */
> - if (bitoff%NBBY) {
> - /* no - bit copy */
> - for (bit=0; bit<nbits; bit++)
> - setbit(out, bit+bitoff, getbit(in, bit));
> - } else {
> - /* yes - byte copy */
> - memcpy(out+byteize(bitoff), in, byteize(nbits));
> - }
> -
> - } else {
> - int ibit;
> - int obit;
> -
> - /* we need to endian swap this value */
> -
> - out+=byteize(bitoff);
> - obit=bitoffs(bitoff);
> -
> - ibit=nbits-NBBY;
> -
> - for (bit=0; bit<nbits; bit++) {
> - setbit(out, bit+obit, getbit(in, ibit));
> - if (ibit%NBBY==NBBY-1)
> - ibit-=NBBY*2-1;
> - else
> - ibit++;
> - }
> - }
> + char *in = ibuf;
> + char *out = obuf;
> + int bit;
> +
> + if (bitoff % NBBY || nbits % NBBY) {
> + for (bit=0; bit < nbits; bit++)
Still whitespace damaged. I'll fix it when I commit it.
> +/*
> + * 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().
Comment is now wrong. I'll fix it when I commit it.
> + 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;
Whitespace still broken. I'll fix it when I commit it.
>
> 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';
You didn't update this like I asked. I'll fix it when I commit it.
> @@ -496,48 +519,77 @@ convert_arg(
> *
> * (but if it starts with "-" assume it's just an integer)
> */
> - int bytes=bit_length/8;
> + int bytes = bit_length / NBBY;
> +
> + /* is this an array of hec numbers? */
> + if (bit_length % NBBY)
> + return NULL;
>
> /* skip leading hash */
> - if (*arg=='#') arg++;
> + if (*arg == '#') arg++;
You didn't update this like I asked. I'll fix it when I commit it.
> + /*
> + * Align the array to point to the field in the array.
> + * rbuf = |MMmm|mmll|ll00|
> + */
> + offset = sizeof(__be64) - 1 - ((bit_length - 1)/ sizeof(__be64));
More whitespace that I've previously pointed out. I'll fix it when I
commit it.
The fix works, so I'll say:
Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
with the caveat that, as the maintainer, I'm going just going to fix
the whitespace problems I've pointed out twice now because I just
want the bug fixed ASAP.
Mark, please take more care in future to address all the review
comments that are made.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
