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

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

[Mark Tinguely]

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

Setting the directory startoff, startblock, and blockcount
bit fields is difficult on both big and little endian machines.
The setting of extentflag bit field 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 these 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 out field is aligned to a byte
boundary but was not shifted correctly.

Convert the input to big endian on little endian machines and
bit/byte shift on all platforms so setbitval can set the bits
correctly. As noted in the comment, the bit shift must be done
before doing the endian conversion or end result will be shifted
in the wrong direction..

Clean some whitespace while in the setbitbal() function.

Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
 db/bit.c   |   68 ++++++++++++++++---------------------------------------------
 db/write.c |   25 +++++++++++++---------
 2 files changed, 33 insertions(+), 60 deletions(-)

Index: b/db/bit.c
===================================================================
--- a/db/bit.c
+++ b/db/bit.c
@@ -130,55 +130,23 @@ getbitval(
 
 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,		/* buffer to write into */
+	int	bitoff,		/* bit offset of where to write */
+	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	= (char *)ibuf;
+	char	*out	= (char *)obuf;
+	int	bit;
+
+	/*
+	 * The input data is in big endian and aligned to the bit length.
+	 * Set the individual bits if the destination field or the source
+	 * end are not aligned.
+	 */
+	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
@@ -451,6 +451,7 @@ convert_arg(
 	int alloc_size;
 	char *ostr;
 	int octval, ret;
+	int offadj;
 
 	if (bit_length <= 64)
 		alloc_size = 8;
@@ -526,16 +527,20 @@ convert_arg(
 		 */
 		*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
+		/*
+		 * Align the significant bits in the result length.
+		 * Must be done before the endian conversion.
+		 */
+		offadj = bit_length % NBBY;
+		if (offadj)
+			*value <<= (8 - offadj);
+
+		/* convert to big endian */
+		*value = cpu_to_be64(*value);
+
+		/* Align the signifant bytes in the result length. */
+		offadj = 7 - (bit_length - 1)/ 8;
+		rbuf += offadj;
 		return rbuf;
 	}
 }


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

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

[Dave Chinner]

On Fri, Feb 07, 2014 at 04:03:42PM -0600, Mark Tinguely wrote:
> Setting the directory startoff, startblock, and blockcount
> bit fields is difficult on both big and little endian machines.
> The setting of extentflag bit field 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

Can you please write an xfstest for this? The BMBT extent records
are the only structures that have unaligned bit offsets and hence
the only ones that exercise this specific code. Clearly no-one
has needed to write a BMBT record in the past 10 years....

> Since these 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 out field is aligned to a byte
> boundary but was not shifted correctly.
> 
> Convert the input to big endian on little endian machines and
> bit/byte shift on all platforms so setbitval can set the bits
> correctly. As noted in the comment, the bit shift must be done
> before doing the endian conversion or end result will be shifted
> in the wrong direction..

Ok, so while we are touching this code, some documentation
explaining the bit shifting requirements, the format of the return
buffer from convert_args, what parameter format setbitval() expects,
etc. Some ascii art demonstrating the incoming and outgoing buffer
contents for convert_arg would be a great help.

> +	char	*in	= (char *)ibuf;
> +	char	*out	= (char *)obuf;
> +	int	bit;

ibuf/obuf are void *, so don't need casts. Also, whitespace:

	char	*in = ibuf;
	char	*out = obuf;

> +	/*
> +	 * The input data is in big endian and aligned to the bit length.
> +	 * Set the individual bits if the destination field or the source
> +	 * end are not aligned.
> +	 */
> +	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));

This is also rather whitespace challenged.

>  }
> Index: b/db/write.c
> ===================================================================
> --- a/db/write.c
> +++ b/db/write.c
> @@ -451,6 +451,7 @@ convert_arg(
>  	int alloc_size;
>  	char *ostr;
>  	int octval, ret;
> +	int offadj;

Just "offset" is sufficient here, and with the scope of use, it
can be declared in the else branch where it is used.

>  
>  	if (bit_length <= 64)
>  		alloc_size = 8;
> @@ -526,16 +527,20 @@ convert_arg(
>  		 */
>  		*value = strtoll(arg, NULL, 0);

If we are touching this code, the return value here should be error
checked.

xfs_db> write u3.bmx[0].startblock 3rgfdw
u3.bmx[0].startblock = 52776558133248
xfs_db> write u3.bmx[0].startblock x3rgfdw
u3.bmx[0].startblock = 0
xfs_db>

i.e. it accepts garbage rather than erroring out.

> -#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
> +		/*
> +		 * Align the significant bits in the result length.
> +		 * Must be done before the endian conversion.
> +		 */
> +		offadj = bit_length % NBBY;
> +		if (offadj)
> +			*value <<= (8 - offadj);

So it gets shifted up by a number 1-7 bits to align the first bit of
the range to a byte boundary. So, that magic "8" should be:

			*value <<= NBBY - offadj;

Ascii art to help readers. Before (host order):

	rbuf
	+---+---+---+---+---+--n+nnn+nnn+
		               + bitlen +
After (host order):
	+---+---+---+---+---+nnn+nnn+n--+
			    + bitlen +
then

> +
> +		/* convert to big endian */
> +		*value = cpu_to_be64(*value);
> +
> +		/* Align the signifant bytes in the result length. */
> +		offadj = 7 - (bit_length - 1)/ 8;
> +		rbuf += offadj;

So the buffer pointer get moved forward by a certain number of bytes
to point at the first byte of the 64 bit big endian value. IOWs, the
magic "7" is actually (sizeof(__be64) - 1) and the "8" is
"sizeof(__be64)" because we are talking about adjusting the offset
within a __be64 variable. IOWs the calculation should be:

		offset = sizeof(__be64) - 1 -
				((bit_length - 1) / sizeof(__be64));

Ascii art to help readers. Before (big endian):

	rbuf
	+---+---+---+---+---+nnn+nnn+n--+
			    + bitlen +

After (big endian):
		            rbuf
                            +nnn+nnn+n--+
			    + bitlen +

And a similar diagram should be added to setbitval to describe the
format of the bit information required in @ibuf.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Mark Tinguely]

On 02/07/14 16:33, Dave Chinner wrote:
> On Fri, Feb 07, 2014 at 04:03:42PM -0600, Mark Tinguely wrote:
>> Setting the directory startoff, startblock, and blockcount
>> bit fields is difficult on both big and little endian machines.
>> The setting of extentflag bit field 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
>
> Can you please write an xfstest for this? The BMBT extent records
> are the only structures that have unaligned bit offsets and hence
> the only ones that exercise this specific code. Clearly no-one
> has needed to write a BMBT record in the past 10 years....

nod. I manually ran through the values valid values for the fields to 
test. A test would be nice.

>> Since these 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 out field is aligned to a byte
>> boundary but was not shifted correctly.
>>
>> Convert the input to big endian on little endian machines and
>> bit/byte shift on all platforms so setbitval can set the bits
>> correctly. As noted in the comment, the bit shift must be done
>> before doing the endian conversion or end result will be shifted
>> in the wrong direction..
>
> Ok, so while we are touching this code, some documentation
> explaining the bit shifting requirements, the format of the return
> buffer from convert_args, what parameter format setbitval() expects,
> etc. Some ascii art demonstrating the incoming and outgoing buffer
> contents for convert_arg would be a great help.
>

okay.

>> +	char	*in	= (char *)ibuf;
>> +	char	*out	= (char *)obuf;
>> +	int	bit;
>
> ibuf/obuf are void *, so don't need casts. Also, whitespace:
>
> 	char	*in = ibuf;
> 	char	*out = obuf;
>
>> +	/*
>> +	 * The input data is in big endian and aligned to the bit length.
>> +	 * Set the individual bits if the destination field or the source
>> +	 * end are not aligned.
>> +	 */
>> +	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));
>
> This is also rather whitespace challenged.

Nod, did not see that.

>
>>   }
>> Index: b/db/write.c
>> ===================================================================
>> --- a/db/write.c
>> +++ b/db/write.c
>> @@ -451,6 +451,7 @@ convert_arg(
>>   	int alloc_size;
>>   	char *ostr;
>>   	int octval, ret;
>> +	int offadj;
>
> Just "offset" is sufficient here, and with the scope of use, it
> can be declared in the else branch where it is used.
>
>>
>>   	if (bit_length<= 64)
>>   		alloc_size = 8;
>> @@ -526,16 +527,20 @@ convert_arg(
>>   		 */
>>   		*value = strtoll(arg, NULL, 0);
>
> If we are touching this code, the return value here should be error
> checked.
>
> xfs_db>  write u3.bmx[0].startblock 3rgfdw
> u3.bmx[0].startblock = 52776558133248

hmm, It should stop at 3. I will take a look.

> xfs_db>  write u3.bmx[0].startblock x3rgfdw
> u3.bmx[0].startblock = 0
> xfs_db>
>
> i.e. it accepts garbage rather than erroring out.

as does all the other writes ...
xfs_db>  write core.nblocks x3rgfdw
core.nblocks = 0

Fixing convert_arg() is beyond the scope of just this patch.

>> -#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
>> +		/*
>> +		 * Align the significant bits in the result length.
>> +		 * Must be done before the endian conversion.
>> +		 */
>> +		offadj = bit_length % NBBY;
>> +		if (offadj)
>> +			*value<<= (8 - offadj);
>
> So it gets shifted up by a number 1-7 bits to align the first bit of
> the range to a byte boundary. So, that magic "8" should be:
>
> 			*value<<= NBBY - offadj;
>
> Ascii art to help readers. Before (host order):
>
> 	rbuf
> 	+---+---+---+---+---+--n+nnn+nnn+
> 		               + bitlen +
> After (host order):
> 	+---+---+---+---+---+nnn+nnn+n--+
> 			    + bitlen +
> then
>
>> +
>> +		/* convert to big endian */
>> +		*value = cpu_to_be64(*value);
>> +
>> +		/* Align the signifant bytes in the result length. */
>> +		offadj = 7 - (bit_length - 1)/ 8;
>> +		rbuf += offadj;
>
> So the buffer pointer get moved forward by a certain number of bytes
> to point at the first byte of the 64 bit big endian value. IOWs, the
> magic "7" is actually (sizeof(__be64) - 1) and the "8" is
> "sizeof(__be64)" because we are talking about adjusting the offset
> within a __be64 variable. IOWs the calculation should be:
>
> 		offset = sizeof(__be64) - 1 -
> 				((bit_length - 1) / sizeof(__be64));
>
> Ascii art to help readers. Before (big endian):
>
> 	rbuf
> 	+---+---+---+---+---+nnn+nnn+n--+
> 			    + bitlen +
>
> After (big endian):
> 		            rbuf
>                              +nnn+nnn+n--+
> 			    + bitlen +
>
> And a similar diagram should be added to setbitval to describe the
> format of the bit information required in @ibuf.
>

Yep, good idea.

--Mark.

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

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

[Dave Chinner]

On Fri, Feb 07, 2014 at 05:13:21PM -0600, Mark Tinguely wrote:
> On 02/07/14 16:33, Dave Chinner wrote:
> >On Fri, Feb 07, 2014 at 04:03:42PM -0600, Mark Tinguely wrote:
> >>@@ -526,16 +527,20 @@ convert_arg(
> >>  		 */
> >>  		*value = strtoll(arg, NULL, 0);
> >
> >If we are touching this code, the return value here should be error
> >checked.
> >
> >xfs_db>  write u3.bmx[0].startblock 3rgfdw
> >u3.bmx[0].startblock = 52776558133248
> 
> hmm, It should stop at 3. I will take a look.

Even if it stops at 3, that's still wrong because it's failed to
process the entire user input....

> >xfs_db>  write u3.bmx[0].startblock x3rgfdw
> >u3.bmx[0].startblock = 0
> >xfs_db>
> >
> >i.e. it accepts garbage rather than erroring out.
> 
> as does all the other writes ...
> xfs_db>  write core.nblocks x3rgfdw
> core.nblocks = 0
> 
> Fixing convert_arg() is beyond the scope of just this patch.

Sure, but I'm not asking you to fix all of convert_args in this
patch, just asking you to do a complete job of fixing the bitval
input processing in this patch.

But, seeing as you've raised that convert_args() has other broken
paths, can you also write new patches to address those issues? It
won't take you long while all this code is fresh in your mind, and
if you do it now it won't get dropped on the floor until somebody
else hits it a couple of years down the track...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Mark Tinguely]

On 02/08/14 02:30, Dave Chinner wrote:
> On Fri, Feb 07, 2014 at 05:13:21PM -0600, Mark Tinguely wrote:
>> On 02/07/14 16:33, Dave Chinner wrote:
>>> On Fri, Feb 07, 2014 at 04:03:42PM -0600, Mark Tinguely wrote:
>>>> @@ -526,16 +527,20 @@ convert_arg(
>>>>   		 */
>>>>   		*value = strtoll(arg, NULL, 0);
>>>
>>> If we are touching this code, the return value here should be error
>>> checked.
>>>
>>> xfs_db>   write u3.bmx[0].startblock 3rgfdw
>>> u3.bmx[0].startblock = 52776558133248
>>
>> hmm, It should stop at 3. I will take a look.
>
> Even if it stops at 3, that's still wrong because it's failed to
> process the entire user input....
>
>>> xfs_db>   write u3.bmx[0].startblock x3rgfdw
>>> u3.bmx[0].startblock = 0
>>> xfs_db>
>>>
>>> i.e. it accepts garbage rather than erroring out.
>>
>> as does all the other writes ...
>> xfs_db>   write core.nblocks x3rgfdw
>> core.nblocks = 0
>>
>> Fixing convert_arg() is beyond the scope of just this patch.
>
> Sure, but I'm not asking you to fix all of convert_args in this
> patch, just asking you to do a complete job of fixing the bitval
> input processing in this patch.
>
> But, seeing as you've raised that convert_args() has other broken
> paths, can you also write new patches to address those issues? It
> won't take you long while all this code is fresh in your mind, and
> if you do it now it won't get dropped on the floor until somebody
> else hits it a couple of years down the track...
>

It needs to be split up. write_string() needs string inputs, 
write_struct() need numeric inputs.  Who uses the UUID-style hex blocks? 
It feels like a black hole of time.

It goes on my 'off the clock' to do list.

--Mark.

--Mark.

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

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

[Dave Chinner]

On Sat, Feb 08, 2014 at 11:30:50AM -0600, Mark Tinguely wrote:
> On 02/08/14 02:30, Dave Chinner wrote:
> >On Fri, Feb 07, 2014 at 05:13:21PM -0600, Mark Tinguely wrote:
> >>On 02/07/14 16:33, Dave Chinner wrote:
> >>>On Fri, Feb 07, 2014 at 04:03:42PM -0600, Mark Tinguely wrote:
> >>>>@@ -526,16 +527,20 @@ convert_arg(
> >>>>  		 */
> >>>>  		*value = strtoll(arg, NULL, 0);
> >>>
> >>>If we are touching this code, the return value here should be error
> >>>checked.
> >>>
> >>>xfs_db>   write u3.bmx[0].startblock 3rgfdw
> >>>u3.bmx[0].startblock = 52776558133248
> >>
> >>hmm, It should stop at 3. I will take a look.
> >
> >Even if it stops at 3, that's still wrong because it's failed to
> >process the entire user input....
> >
> >>>xfs_db>   write u3.bmx[0].startblock x3rgfdw
> >>>u3.bmx[0].startblock = 0
> >>>xfs_db>
> >>>
> >>>i.e. it accepts garbage rather than erroring out.
> >>
> >>as does all the other writes ...
> >>xfs_db>   write core.nblocks x3rgfdw
> >>core.nblocks = 0
> >>
> >>Fixing convert_arg() is beyond the scope of just this patch.
> >
> >Sure, but I'm not asking you to fix all of convert_args in this
> >patch, just asking you to do a complete job of fixing the bitval
> >input processing in this patch.
> >
> >But, seeing as you've raised that convert_args() has other broken
> >paths, can you also write new patches to address those issues? It
> >won't take you long while all this code is fresh in your mind, and
> >if you do it now it won't get dropped on the floor until somebody
> >else hits it a couple of years down the track...
> >
> 
> It needs to be split up. write_string() needs string inputs,
> write_struct() need numeric inputs.

write_struct() handles every type of possible input -
string varables, integers, UUIDs, etc. i.e. something like

xfs_db> write sb.uuid <xxxx-yyyy-....>

runs through write_struct(), it finds the uuid field definition in
the superblock description and then calls convert_args() on it to
convert the uuid format input.

IOWs, convert_args() needs to handle all the types of input. Hence
you might like to factor convert_args, but it still needs to handle
arbitrary input types.

>  Who uses the UUID-style hex
> blocks? It feels like a black hole of time.

Obvious answer: UUIDs

$ git grep FLDT_UUID
db/agf.c:       { "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
db/agfl.c:      { "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
db/agi.c:       { "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
db/btblock.c:   { "uuid", FLDT_UUID, OI(OFF(u.l.bb_uuid)), C1, 0, TYP_NONE },
db/btblock.c:   { "uuid", FLDT_UUID, OI(OFF(u.l.bb_uuid)), C1, 0, TYP_NONE },
db/btblock.c:   { "uuid", FLDT_UUID, OI(OFF(u.s.bb_uuid)), C1, 0, TYP_NONE },
db/btblock.c:   { "uuid", FLDT_UUID, OI(OFF(u.s.bb_uuid)), C1, 0, TYP_NONE },
db/btblock.c:   { "uuid", FLDT_UUID, OI(OFF(u.s.bb_uuid)), C1, 0, TYP_NONE },
db/dir2.c:      { "uuid", FLDT_UUID, OI(DBH3OFF(uuid)), C1, 0, TYP_NONE },
db/dir2.c:      { "uuid", FLDT_UUID, OI(DB3OFF(uuid)), C1, 0, TYP_NONE },
db/dquot.c:     { "uuid", FLDT_UUID, OI(DDOFF(uuid)), C1, 0, TYP_NONE },
db/field.c:     { FLDT_UUID, "uuid", fp_uuid, NULL, SI(bitsz(uuid_t)), 0, NULL, N
db/field.h:     FLDT_UUID,
db/inode.c:     { "uuid", FLDT_UUID, OI(COFF(uuid)), C1, 0, TYP_NONE },
db/inode.c:     { "muuid", FLDT_UUID, NULL, inode_u_muuid_count, FLD_COUNT, TYP_N
db/sb.c:        { "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
db/symlink.c:   { "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
$

Slightly less obvious answer is writing arbitrary binary data to
block regions in structures, such as writing multiple entries in an
array in a single command (e.g. FLD_ARRAY structure sections). I've
used this to move sections of directory blocks around in the past....

> It goes on my 'off the clock' to do list.

Ok, I'll just assume that means it'll never get done so I'll get a
pleasent surprise when you send the patch tomorrow....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Mark Tinguely]

On 02/09/14 17:22, Dave Chinner wrote:
> On Sat, Feb 08, 2014 at 11:30:50AM -0600, Mark Tinguely wrote:
>> On 02/08/14 02:30, Dave Chinner wrote:
>>> On Fri, Feb 07, 2014 at 05:13:21PM -0600, Mark Tinguely wrote:
>>>> On 02/07/14 16:33, Dave Chinner wrote:
>>>>> On Fri, Feb 07, 2014 at 04:03:42PM -0600, Mark Tinguely wrote:
>>>>>> @@ -526,16 +527,20 @@ convert_arg(
>>>>>>   		 */
>>>>>>   		*value = strtoll(arg, NULL, 0);
>>>>>
>>>>> If we are touching this code, the return value here should be error
>>>>> checked.
>>>>>
>>>>> xfs_db>    write u3.bmx[0].startblock 3rgfdw
>>>>> u3.bmx[0].startblock = 52776558133248
>>>>
>>>> hmm, It should stop at 3. I will take a look.
>>>
>>> Even if it stops at 3, that's still wrong because it's failed to
>>> process the entire user input....
>>>
>>>>> xfs_db>    write u3.bmx[0].startblock x3rgfdw
>>>>> u3.bmx[0].startblock = 0
>>>>> xfs_db>
>>>>>
>>>>> i.e. it accepts garbage rather than erroring out.
>>>>
>>>> as does all the other writes ...
>>>> xfs_db>    write core.nblocks x3rgfdw
>>>> core.nblocks = 0
>>>>
>>>> Fixing convert_arg() is beyond the scope of just this patch.
>>>
>>> Sure, but I'm not asking you to fix all of convert_args in this
>>> patch, just asking you to do a complete job of fixing the bitval
>>> input processing in this patch.
>>>
>>> But, seeing as you've raised that convert_args() has other broken
>>> paths, can you also write new patches to address those issues? It
>>> won't take you long while all this code is fresh in your mind, and
>>> if you do it now it won't get dropped on the floor until somebody
>>> else hits it a couple of years down the track...
>>>
>>
>> It needs to be split up. write_string() needs string inputs,
>> write_struct() need numeric inputs.
>
> write_struct() handles every type of possible input -
> string varables, integers, UUIDs, etc. i.e. something like
>
> xfs_db>  write sb.uuid<xxxx-yyyy-....>
>
> runs through write_struct(), it finds the uuid field definition in
> the superblock description and then calls convert_args() on it to
> convert the uuid format input.
>
> IOWs, convert_args() needs to handle all the types of input. Hence
> you might like to factor convert_args, but it still needs to handle
> arbitrary input types.
>
>>   Who uses the UUID-style hex
>> blocks? It feels like a black hole of time.
>
> Obvious answer: UUIDs
>
> $ git grep FLDT_UUID
> db/agf.c:       { "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
> db/agfl.c:      { "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
> db/agi.c:       { "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
> db/btblock.c:   { "uuid", FLDT_UUID, OI(OFF(u.l.bb_uuid)), C1, 0, TYP_NONE },
> db/btblock.c:   { "uuid", FLDT_UUID, OI(OFF(u.l.bb_uuid)), C1, 0, TYP_NONE },
> db/btblock.c:   { "uuid", FLDT_UUID, OI(OFF(u.s.bb_uuid)), C1, 0, TYP_NONE },
> db/btblock.c:   { "uuid", FLDT_UUID, OI(OFF(u.s.bb_uuid)), C1, 0, TYP_NONE },
> db/btblock.c:   { "uuid", FLDT_UUID, OI(OFF(u.s.bb_uuid)), C1, 0, TYP_NONE },
> db/dir2.c:      { "uuid", FLDT_UUID, OI(DBH3OFF(uuid)), C1, 0, TYP_NONE },
> db/dir2.c:      { "uuid", FLDT_UUID, OI(DB3OFF(uuid)), C1, 0, TYP_NONE },
> db/dquot.c:     { "uuid", FLDT_UUID, OI(DDOFF(uuid)), C1, 0, TYP_NONE },
> db/field.c:     { FLDT_UUID, "uuid", fp_uuid, NULL, SI(bitsz(uuid_t)), 0, NULL, N
> db/field.h:     FLDT_UUID,
> db/inode.c:     { "uuid", FLDT_UUID, OI(COFF(uuid)), C1, 0, TYP_NONE },
> db/inode.c:     { "muuid", FLDT_UUID, NULL, inode_u_muuid_count, FLD_COUNT, TYP_N
> db/sb.c:        { "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
> db/symlink.c:   { "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
> $
>
> Slightly less obvious answer is writing arbitrary binary data to
> block regions in structures, such as writing multiple entries in an
> array in a single command (e.g. FLD_ARRAY structure sections). I've
> used this to move sections of directory blocks around in the past....
>
>> It goes on my 'off the clock' to do list.
>
> Ok, I'll just assume that means it'll never get done so I'll get a
> pleasent surprise when you send the patch tomorrow....
>
> Cheers,
>
> Dave.

Yes, realized that the types are all needed:
  write core.magic "IN"
does make sense. And the following is legal:
  write core.mode "IN"
but may not make a lot of sense.

My patch broke on little endian the ability to input 8-64 bit hex values:

   write core.magic #494e

it was always broke on big endian, so I fixed it for both.

I cleaned up the trailing junk characters.

The test has been written and it will be reposted once I get to the 
documentation changes.

--Mark.

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