[PATCH v2] xfs_db: fix the setting of unaligned directory fields

[PATCH v2] xfs_db: fix the setting of unaligned directory fields

[Mark Tinguely]

[-- Attachment #1: xfs_db-fix-dir-settings.patch --]
[-- Type: text/plain, Size: 9914 bytes --]

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.

Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
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 |  173 +++++++++++++++++++++++++++++++++++++++++--------------------
 2 files changed, 152 insertions(+), 105 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++)
+			setbit(out, bit+bitoff, getbit(in, bit));
+	} else
+		memcpy(out+byteize(bitoff), in, byteize(nbits));
 }
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;
 
 	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';
 
-		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++;
 
-		    /* 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;
+	} 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;
+	/*
+	 * 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));
+	rbuf += offset;
+	return rbuf;
 }
 
 
@@ -550,9 +613,9 @@ write_struct(
 {
 	const ftattr_t	*fa;
 	flist_t		*fl;
-	flist_t         *sfl;
-	int             bit_length;
-	char            *buf;
+	flist_t		*sfl;
+	int		bit_length;
+	char		*buf;
 	int		parentoffset;
 
 	if (argc != 2) {


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs_db: fix the setting of unaligned directory fields

[Dave Chinner]

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@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs_db: fix the setting of unaligned directory fields

[Mark Tinguely]

On 02/10/14 19:31, Dave Chinner wrote:
> On Mon, Feb 10, 2014 at 05:09:12PM -0600, Mark Tinguely wrote:

<delete>

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

The comment explains the return.

If the input bit_length is bigger than 64 bit then it is an array of hex 
numbers (for example UUID can be entered this way).

buf is the beginning of the allocated array before rbuf pointer put 
nibbles into it. So this returns the beginning of the hex bytes.

If this return is not taken then it is 64 bits or less and is some kind 
of integer. The integer will get its fields fixed and converted to big 
endian....

--Mark.


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs_db: fix the setting of unaligned directory fields

[Dave Chinner]

On Tue, Feb 11, 2014 at 08:18:41AM -0600, Mark Tinguely wrote:
> On 02/10/14 19:31, Dave Chinner wrote:
> >On Mon, Feb 10, 2014 at 05:09:12PM -0600, Mark Tinguely wrote:
> 
> <delete>
> 
> >>>  +		 * 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....
> 
> The comment explains the return.

If the comment answered my question then I wouldn't have asked it.

> If the input bit_length is bigger than 64 bit then it is an array of
> hex numbers (for example UUID can be entered this way).

Yes, I know that.

> buf is the beginning of the allocated array before rbuf pointer put
> nibbles into it. So this returns the beginning of the hex bytes.

Yes, I know that, too.

> If this return is not taken then it is 64 bits or less and is some
> kind of integer. The integer will get its fields fixed and converted
> to big endian....

Oh, now I understand what you've done - you've changed the
definition of what a hex block contains. A hex block is a
hex-encoded binary format - it is just an array of bytes. What
you've changed is that you now define a hex block of less than 64
bits to encode a host-endian format integer.

That's what you comment didn't explain and what confused me - why we
should be bit shifting or endian converting a raw data encoding.

For example, if I want to write 4 bytes of hex data to a field, then
I use "#12345678" to do it. When I dump the raw contents of that
field, I expect to see four bytes in big endian format in that
field: 12 34 56 78. When I print the structure value, I expect to
see the host-endian conversion of that raw data, not the on-disk
encoded value.

IOWs, we do not want small hex block arrays to be interpretted as a
host endian integer - if you need to enter an integer value in host
endian then use "0x12345678". i.e. hex blocks are not integers. i.e.

xfs_db> write lsn #12345678
lsn = 0x78563412
xfs_db> write lsn 0x12345678
lsn = 0x12345678

The above behaviour is correct and expected on a little endian host.
Having them both result in "lsn = 0x12345678" is not the correct or
desired behaviour. I didn't pick this up when looking that the tst
you wrote, but it's now obvious:

+$XFS_DB_PROG -x -c "inode $FILE_INO" -c "write core.gen #185a" $SCRATCH_DEV
+$XFS_DB_PROG -x -c "inode $FILE_INO" -c "write core.gen #a518" $SCRATCH_DEV
...
+core.gen = 6234
+core.gen = 42264

This is how a little endian host should behave:

xfs_db> write core.gen 0xa518
core.gen = 42264
xfs_db> write core.gen #a518
core.gen = 6309
xfs_db> write core.gen 0x18a5
core.gen = 6309
xfs_db> write core.gen #18a5
core.gen = 42264


So, back to my original question, now I undersand what you've done,
which was "how does hexblock data on unaligned bit_length fields
work?". The code ends up like this:

		int bytes = bit_length / NBBY;

		while (*arg && bytes--) {
			.....
		}

		if (bytes < 0 && *arg)
			return NULL;

		/*
		 * Values larger than 64 bits are array of hex digits that
		 * already in the desired ordering (example UUID).
		 */
		if (bit_length > 64)
		     return buf;
	} else {
		....
	}

	/* do integer parsing */
	......

Let's say bit length is 1 (e.g. unwritten extent flag) and we pass a
value of "#1". That gives us bytes = 0 as the initial condition.
This:

	while (*arg && bytes--)

sees *arg != NULL and bytes = 0 so it doesn't run the loop and
then post-decrements bytes Now we have bytes = -1 and *arg != 0.
So we return NULL instead of parsing the byte.

The same thing will happen with any unaligned bit field that needs
the high byte set via this code as it will fail to parse the last
character of the input.

IOWs, using hexblock format on unaligned bitfields does not work
properly. In fact, it's never been supported, as hex blocks
were never intended to be used that way. i.e. the use for a hexblock
when writting an extent record to to write the entire record as a
binary value, not to write individual elements as integer values....

Hence I'd suggest that "if (bit_field & NBBY) return NULL;" is
appropriate for hex block format input, and the input should never
be treated as a host-endian integer...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs_db: fix the setting of unaligned directory fields

[Mark Tinguely]

On 02/11/14 18:22, Dave Chinner wrote:
> On Tue, Feb 11, 2014 at 08:18:41AM -0600, Mark Tinguely wrote:
>> On 02/10/14 19:31, Dave Chinner wrote:

...

> Hence I'd suggest that "if (bit_field & NBBY) return NULL;" is
> appropriate for hex block format input, and the input should never
> be treated as a host-endian integer...
>
> Cheers,
>
> Dave.

I don't like having the hex block format as an integer input. My change 
would change the result if used as an integer, I just trying to keep 
compatibility with the previous code.

I will gladly leave the hex block input alone and remove the test that 
uses it as an integer.

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v3] xfs_db: fix the setting of unaligned directory fields

[Mark Tinguely]

[-- Attachment #1: xfs_db-fix-dir-settings.patch --]
[-- Type: text/plain, Size: 9635 bytes --]

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@sgi.com>
---
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++)
+			setbit(out, bit + bitoff, getbit(in, bit));
+	} else
+		memcpy(out + byteize(bitoff), in, byteize(nbits));
 }
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;
 
 	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';
 
-		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
@@ -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++;
 
 		while (*arg && bytes--) {
-		    /* skip hypens */
-		    while (*arg=='-') arg++;
+			/* skip hypens */
+			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;
+			*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++;
 		}
-		if (bytes<0&&*arg) return NULL;
+		if (bytes < 0 && *arg)
+			return NULL;
+
 		return buf;
-	} else {
-		/*
-		 * handle integers
-		 */
-		*value = strtoll(arg, NULL, 0);
+	} 
 
-#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;
-	}
+	/* handle decimal / hexadecimal integers */
+	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) > 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));
+	rbuf += offset;
+	return rbuf;
 }
 
 
@@ -550,9 +602,9 @@ write_struct(
 {
 	const ftattr_t	*fa;
 	flist_t		*fl;
-	flist_t         *sfl;
-	int             bit_length;
-	char            *buf;
+	flist_t		*sfl;
+	int		bit_length;
+	char		*buf;
 	int		parentoffset;
 
 	if (argc != 2) {


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3] xfs_db: fix the setting of unaligned directory fields

[Dave Chinner]

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@sgi.com>
> ---
> 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@redhat.com>

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@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs