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@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
