Message ID | 20181026201943.24131-1-stefanrin@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | v4 Try to squash metadump data leaks | expand |
On 10/26/18 3:19 PM, Stefan Ring wrote: > I reorganized the patches as suggested and fixed the btree case. > > Stefan Ring (5): > xfs_metadump: Extend data zapping to XFS_DIR{2,3}_LEAFN_MAGIC blocks > xfs_metadump: Zap multi fsb blocks > xfs_metadump: Zap freeindex blocks in directory inodes > xfs_metadump: Zap unused space in inode btrees > xfs_metadump: Zap dev inodes > > db/metadump.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 105 insertions(+), 14 deletions(-) > Cool, thanks for splitting these up, will review & test in earnest. These also need your: Signed-off-by: Stefan Ring <stefanrin@gmail.com> But there may be a v5 yet, so stash that away for now :) -Eric
Also, here's an old script I had lying around to test metadump. It's hacky, sorry. Things to watch out for... it looks for an "fsstress" binary from xfstests, so adjust that path. It freezes and unfreezes the test filesystem, if your mount fails it'll freeze the fs you're on. ;) There may be other rough spots. It also runs the xfs_metadump/xfs_db in your path; you could change that to a local ./xfs_metadump to run db/xfs_db from a git tree instead for testing w/o make install. Right now this is detecting some corruption induced by metadump/mdrestore with your full patchset in place, FWIW. Sorry I didn't send this sooner, kinda forgot I had it. really should turn it into an xfstest. ----- #!/bin/bash function _fail () { echo $1 exit 1 } FSSTRESS=/root/xfstests-dev/ltp/fsstress mkdir -p mnt umount mnt &>/dev/null # Will fill fsfile.img with "cd cd cd" echo "Patterning 256M image file" xfs_io -F -f -c "pwrite 0 256m" fsfile.img &>/dev/null # Make & label the filesystem, and mount it. echo "mkfs & label the image, and mount it" mkfs.xfs -b size=2048 -m crc=0 -L "fslabel" fsfile.img mount -o loop fsfile.img mnt cd mnt # Attempt to make files of "every" format for data, dirs, attrs etc. # ====== File Data ====== echo "Creating file types ..." # Regular files # - FMT_EXTENTS touch S_IFREG.FMT_EXTENTS xfs_io -c "pwrite 0 4k" S_IFREG.FMT_EXTENTS &>/dev/null # - FMT_BTREE touch S_IFREG.FMT_BTREE for I in `seq 0 8 200`; do xfs_io -d -c "pwrite ${I}k 4k" S_IFREG.FMT_BTREE &>/dev/null done # ======= Directories ======= echo "Creating directory types ..." # - FMT_LOCAL mkdir S_IFDIR.FMT_LOCAL touch S_IFDIR.FMT_LOCAL/localdirfile # - FMT_EXTENTS mkdir S_IFDIR.FMT_EXTENTS for I in `seq 1 100`; do touch S_IFDIR.FMT_EXTENTS/extent_dir_file_$I done # With a few missing for I in `seq 10 2 20` 100; do rm -f S_IFDIR.FMT_EXTENTS/extent_dir_file_$I done # - FMT_BTREE mkdir S_IFDIR.FMT_BTREE for I in `seq 1 1000`; do touch S_IFDIR.FMT_BTREE/btree_dir_file_$I done # With a few missing for I in `seq 10 2 20` 1000; do rm -f S_IFDIR.FMT_BTREE/btree_dir_file_$I done # Dave's special hack - grow freespace tree mkdir S_IFDIR.FMT_BTREE2 for I in `seq 1 5000`; do touch S_IFDIR.FMT_BTREE2/btree2_dir_file_$I done # Remove every other for I in `seq 1 2 5000`; do rm -f S_IFDIR.FMT_BTREE2/btree2_dir_file_$I done # ======= Symlinks ======= echo "Creating symlink types ..." # - FMT_LOCAL ln -s target S_IFLNK.FMT_LOCAL # - FMT_EXTENTS # create "strangely_long_path_component/strangely_long_path_component/..." COMP=strangely_long_path_component TARGET=$COMP for I in `seq 1 30`; do TARGET=$TARGET/$COMP done ln -s $TARGET S_IFLNK.FMT_EXTENTS # ======= Char & block devices ======= echo "Creating char & block types ..." mkdir S_IFDIR.DEVICES mknod S_IFDIR.DEVICES/S_IFCHR c 1 1 mknod S_IFDIR.DEVICES/S_IFBLK c 1 1 # Create an inode with some local data & then remove echo "Create local symlink" touch S_IFDIR.DEVICES/longnamenamenamenamenamenamenamenamenamename ln -s "longnamenamenamenamenamenamenamenamenamename" S_IFDIR.DEVICES/link xfs_io -c fsync S_IFDIR.DEVICES/link rm -f S_IFDIR.DEVICES/link mknod S_IFDIR.DEVICES/S_IFBLK2 c 1 1 # ======= Attributes ======= echo "Creating attribute types ..." # FMT_LOCAL touch S_IFREG.ATTR.FMT_LOCAL setfattr -n user.localattrname -v localattrvalue S_IFREG.ATTR.FMT_LOCAL # FMT_EXTENTS touch S_IFREG.ATTR.FMT_EXTENTS for I in `seq 1 50`; do setfattr -n user.extentattrname$I -v extentattrvalue S_IFREG.ATTR.FMT_EXTENTS done # With a few missing for I in 10 12 50; do setfattr -x user.extentattrname$I S_IFREG.ATTR.FMT_EXTENTS done # FMT_EXTENTS with a remote 3k value, fill with "C" touch S_IFREG.ATTR.FMT_EXTENTS_REMOTE3K xfs_io -f -c "pwrite -S 0x43 0 3k" S_IFREG.ATTRVALFILE &>/dev/null attr -q -s user.remotebtreeattrname S_IFREG.ATTR.FMT_EXTENTS_REMOTE3K < S_IFREG.ATTRVALFILE # FMT_EXTENTS with a remote 4k value, fill with "D" touch S_IFREG.ATTR.FMT_EXTENTS_REMOTE4K xfs_io -f -c "pwrite -S 0x44 0 4k" S_IFREG.ATTRVALFILE &>/dev/null attr -q -s user.remotebtreeattrname S_IFREG.ATTR.FMT_EXTENTS_REMOTE4K < S_IFREG.ATTRVALFILE # FMT_BTREE touch S_IFREG.ATTR.FMT_BTREE for I in `seq 1 1000`; do setfattr -n user.btreeattrname$I -v btreeattrlongervalue S_IFREG.ATTR.FMT_BTREE done # With a few missing for I in 10 12 1000; do setfattr -x user.btreeattrname$I S_IFREG.ATTR.FMT_BTREE done # Make an unused inode mkdir S_IFDIR.DELETED touch S_IFDIR.DELETED/S_IFREG.DELETED # Really push this to disk xfs_freeze -f . xfs_freeze -u . rm -f S_IFDIR.DELETED/S_IFREG.DELETED # ============================= # Now fsstress for some good randomness mkdir stress echo "fsstressing" $FSSTRESS -d stress -p 4 -n 1000 echo "done" sleep 5 cd - echo "umount & remount" umount mnt mount -o loop fsfile.img mnt # Get details of what's on disk in the original fs echo "Get list of original files & attributes" ls -lR mnt > orig_files getfattr -m - -dR mnt > orig_attrs 2>/dev/null echo "FS utilization:" df mnt/ echo "umount" umount mnt # Test that we didn't lose anything with stale-data-zeroing # turned on (i.e. zap too much) # dump & restore it, repair it, and compare contents to orig # ===== NON-OBFUSCATED ===== # do a NON-obfuscated metadump & look for stale pattern coming through rm -f fsfile-clear.img echo "Non-obfuscated metadump" ./xfs_metadump -o fsfile.img - | xfs_mdrestore - fsfile-clear.img # Make sure it's not corrupt echo "xfs_repair on unobfuscated & stale-zeroed metadump" xfs_repair -n fsfile-clear.img 2>/dev/null || _fail "Repair failed on fsfile-clear.img" # Get details of what's on disk in the image mount -o loop fsfile-clear.img mnt ls -lR mnt > clear_files getfattr -m - -dR mnt > clear_attrs 2>/dev/null echo "Checking for unchanged files & attrs via unobfuscated metadump" diff -u orig_files clear_files diff -u orig_attrs clear_attrs echo "Looking for stale data in unobfuscated dump" # Generic stale data test - look for original pattern hexdump -C fsfile-clear.img | grep "cd cd cd" # ===== OBFUSCATED ===== # Now do OBFUSCATED metadump & look for stale strings coming through, # as well as looking for any other data we wrote rm -f fsfile-obfuscated.img xfs_metadump fsfile.img - | xfs_mdrestore - fsfile-obfuscated.img # Make sure it's not corrupt echo "xfs_repair on obfuscated & stale-zeroed metadump" xfs_repair -n fsfile-obfuscated.img 2>/dev/null || _fail "Repair failed on fsfile-obfuscated.img" # Generic stale data test - look for original pattern echo "Looking for stale data in obfuscated dump" hexdump -C fsfile-obfuscated.img | grep "cd cd cd" # Look for stuff we explicitly wrote echo "Looking for our data in obfuscated dump" strings -t x fsfile-obfuscated.img | grep -i "S_IF\|attr\|name\|value\|btree\|long\|local\|extent\|label"
On Fri, Oct 26, 2018 at 06:33:21PM -0500, Eric Sandeen wrote: > Also, here's an old script I had lying around to test metadump. It's hacky, sorry. > > Things to watch out for... it looks for an "fsstress" binary from xfstests, so adjust that path. > It freezes and unfreezes the test filesystem, if your mount fails it'll freeze > the fs you're on. ;) There may be other rough spots. > > It also runs the xfs_metadump/xfs_db in your path; you could change that to a > local ./xfs_metadump to run db/xfs_db from a git tree instead for testing > w/o make install. > > Right now this is detecting some corruption induced by metadump/mdrestore > with your full patchset in place, FWIW. > > Sorry I didn't send this sooner, kinda forgot I had it. really should > turn it into an xfstest. I did, see xfs/349. It doesn't test metadump tho... maybe it should? (Also see xfs/432...) --D > > ----- > > #!/bin/bash > > function _fail () { > echo $1 > exit 1 > } > > FSSTRESS=/root/xfstests-dev/ltp/fsstress > > mkdir -p mnt > umount mnt &>/dev/null > > # Will fill fsfile.img with "cd cd cd" > echo "Patterning 256M image file" > xfs_io -F -f -c "pwrite 0 256m" fsfile.img &>/dev/null > # Make & label the filesystem, and mount it. > echo "mkfs & label the image, and mount it" > mkfs.xfs -b size=2048 -m crc=0 -L "fslabel" fsfile.img > mount -o loop fsfile.img mnt > > cd mnt > > # Attempt to make files of "every" format for data, dirs, attrs etc. > > # ====== File Data ====== > > echo "Creating file types ..." > # Regular files > # - FMT_EXTENTS > touch S_IFREG.FMT_EXTENTS > xfs_io -c "pwrite 0 4k" S_IFREG.FMT_EXTENTS &>/dev/null > # - FMT_BTREE > touch S_IFREG.FMT_BTREE > for I in `seq 0 8 200`; do > xfs_io -d -c "pwrite ${I}k 4k" S_IFREG.FMT_BTREE &>/dev/null > done > > # ======= Directories ======= > echo "Creating directory types ..." > # - FMT_LOCAL > mkdir S_IFDIR.FMT_LOCAL > touch S_IFDIR.FMT_LOCAL/localdirfile > > # - FMT_EXTENTS > mkdir S_IFDIR.FMT_EXTENTS > for I in `seq 1 100`; do > touch S_IFDIR.FMT_EXTENTS/extent_dir_file_$I > done > # With a few missing > for I in `seq 10 2 20` 100; do > rm -f S_IFDIR.FMT_EXTENTS/extent_dir_file_$I > done > > # - FMT_BTREE > mkdir S_IFDIR.FMT_BTREE > for I in `seq 1 1000`; do > touch S_IFDIR.FMT_BTREE/btree_dir_file_$I > done > # With a few missing > for I in `seq 10 2 20` 1000; do > rm -f S_IFDIR.FMT_BTREE/btree_dir_file_$I > done > > # Dave's special hack - grow freespace tree > mkdir S_IFDIR.FMT_BTREE2 > for I in `seq 1 5000`; do > touch S_IFDIR.FMT_BTREE2/btree2_dir_file_$I > done > # Remove every other > for I in `seq 1 2 5000`; do > rm -f S_IFDIR.FMT_BTREE2/btree2_dir_file_$I > done > > # ======= Symlinks ======= > echo "Creating symlink types ..." > # - FMT_LOCAL > ln -s target S_IFLNK.FMT_LOCAL > # - FMT_EXTENTS > # create "strangely_long_path_component/strangely_long_path_component/..." > COMP=strangely_long_path_component > TARGET=$COMP > for I in `seq 1 30`; do > TARGET=$TARGET/$COMP > done > ln -s $TARGET S_IFLNK.FMT_EXTENTS > > # ======= Char & block devices ======= > echo "Creating char & block types ..." > mkdir S_IFDIR.DEVICES > mknod S_IFDIR.DEVICES/S_IFCHR c 1 1 > mknod S_IFDIR.DEVICES/S_IFBLK c 1 1 > # Create an inode with some local data & then remove > echo "Create local symlink" > touch S_IFDIR.DEVICES/longnamenamenamenamenamenamenamenamenamename > ln -s "longnamenamenamenamenamenamenamenamenamename" S_IFDIR.DEVICES/link > xfs_io -c fsync S_IFDIR.DEVICES/link > rm -f S_IFDIR.DEVICES/link > mknod S_IFDIR.DEVICES/S_IFBLK2 c 1 1 > > # ======= Attributes ======= > echo "Creating attribute types ..." > # FMT_LOCAL > touch S_IFREG.ATTR.FMT_LOCAL > setfattr -n user.localattrname -v localattrvalue S_IFREG.ATTR.FMT_LOCAL > # FMT_EXTENTS > touch S_IFREG.ATTR.FMT_EXTENTS > for I in `seq 1 50`; do > setfattr -n user.extentattrname$I -v extentattrvalue S_IFREG.ATTR.FMT_EXTENTS > done > # With a few missing > for I in 10 12 50; do > setfattr -x user.extentattrname$I S_IFREG.ATTR.FMT_EXTENTS > done > > # FMT_EXTENTS with a remote 3k value, fill with "C" > touch S_IFREG.ATTR.FMT_EXTENTS_REMOTE3K > xfs_io -f -c "pwrite -S 0x43 0 3k" S_IFREG.ATTRVALFILE &>/dev/null > attr -q -s user.remotebtreeattrname S_IFREG.ATTR.FMT_EXTENTS_REMOTE3K < S_IFREG.ATTRVALFILE > > # FMT_EXTENTS with a remote 4k value, fill with "D" > touch S_IFREG.ATTR.FMT_EXTENTS_REMOTE4K > xfs_io -f -c "pwrite -S 0x44 0 4k" S_IFREG.ATTRVALFILE &>/dev/null > attr -q -s user.remotebtreeattrname S_IFREG.ATTR.FMT_EXTENTS_REMOTE4K < S_IFREG.ATTRVALFILE > > # FMT_BTREE > touch S_IFREG.ATTR.FMT_BTREE > for I in `seq 1 1000`; do > setfattr -n user.btreeattrname$I -v btreeattrlongervalue S_IFREG.ATTR.FMT_BTREE > done > # With a few missing > for I in 10 12 1000; do > setfattr -x user.btreeattrname$I S_IFREG.ATTR.FMT_BTREE > done > > # Make an unused inode > mkdir S_IFDIR.DELETED > touch S_IFDIR.DELETED/S_IFREG.DELETED > # Really push this to disk > xfs_freeze -f . > xfs_freeze -u . > rm -f S_IFDIR.DELETED/S_IFREG.DELETED > > # ============================= > > # Now fsstress for some good randomness > > mkdir stress > echo "fsstressing" > $FSSTRESS -d stress -p 4 -n 1000 > echo "done" > > sleep 5 > > cd - > > echo "umount & remount" > umount mnt > mount -o loop fsfile.img mnt > > # Get details of what's on disk in the original fs > echo "Get list of original files & attributes" > ls -lR mnt > orig_files > getfattr -m - -dR mnt > orig_attrs 2>/dev/null > > echo "FS utilization:" > df mnt/ > > echo "umount" > umount mnt > > # Test that we didn't lose anything with stale-data-zeroing > # turned on (i.e. zap too much) > # dump & restore it, repair it, and compare contents to orig > > # ===== NON-OBFUSCATED ===== > # do a NON-obfuscated metadump & look for stale pattern coming through > > rm -f fsfile-clear.img > echo "Non-obfuscated metadump" > ./xfs_metadump -o fsfile.img - | xfs_mdrestore - fsfile-clear.img > # Make sure it's not corrupt > echo "xfs_repair on unobfuscated & stale-zeroed metadump" > xfs_repair -n fsfile-clear.img 2>/dev/null || _fail "Repair failed on fsfile-clear.img" > # Get details of what's on disk in the image > mount -o loop fsfile-clear.img mnt > ls -lR mnt > clear_files > getfattr -m - -dR mnt > clear_attrs 2>/dev/null > > echo "Checking for unchanged files & attrs via unobfuscated metadump" > diff -u orig_files clear_files > diff -u orig_attrs clear_attrs > > echo "Looking for stale data in unobfuscated dump" > # Generic stale data test - look for original pattern > hexdump -C fsfile-clear.img | grep "cd cd cd" > > # ===== OBFUSCATED ===== > # Now do OBFUSCATED metadump & look for stale strings coming through, > # as well as looking for any other data we wrote > rm -f fsfile-obfuscated.img > xfs_metadump fsfile.img - | xfs_mdrestore - fsfile-obfuscated.img > # Make sure it's not corrupt > echo "xfs_repair on obfuscated & stale-zeroed metadump" > xfs_repair -n fsfile-obfuscated.img 2>/dev/null || _fail "Repair failed on fsfile-obfuscated.img" > > # Generic stale data test - look for original pattern > echo "Looking for stale data in obfuscated dump" > hexdump -C fsfile-obfuscated.img | grep "cd cd cd" > > # Look for stuff we explicitly wrote > echo "Looking for our data in obfuscated dump" > strings -t x fsfile-obfuscated.img | grep -i "S_IF\|attr\|name\|value\|btree\|long\|local\|extent\|label" >
On Sat, Oct 27, 2018 at 1:33 AM Eric Sandeen <sandeen@sandeen.net> wrote: > > Also, here's an old script I had lying around to test metadump. It's hacky, sorry. > > Things to watch out for... it looks for an "fsstress" binary from xfstests, so adjust that path. > It freezes and unfreezes the test filesystem, if your mount fails it'll freeze > the fs you're on. ;) There may be other rough spots. > > It also runs the xfs_metadump/xfs_db in your path; you could change that to a > local ./xfs_metadump to run db/xfs_db from a git tree instead for testing > w/o make install. > > Right now this is detecting some corruption induced by metadump/mdrestore > with your full patchset in place, FWIW. > > Sorry I didn't send this sooner, kinda forgot I had it. really should turn it into an xfstest. Thanks! The corruption is caused by the last patch in the series. So dev inodes can have attribute forks. I will have to zap the data and the attr area separately.
On Sun, Oct 28, 2018 at 1:38 PM Stefan Ring <stefanrin@gmail.com> wrote: > > On Sat, Oct 27, 2018 at 1:33 AM Eric Sandeen <sandeen@sandeen.net> wrote: > > > > Also, here's an old script I had lying around to test metadump. It's hacky, sorry. > > > > Things to watch out for... it looks for an "fsstress" binary from xfstests, so adjust that path. > > It freezes and unfreezes the test filesystem, if your mount fails it'll freeze > > the fs you're on. ;) There may be other rough spots. > > > > It also runs the xfs_metadump/xfs_db in your path; you could change that to a > > local ./xfs_metadump to run db/xfs_db from a git tree instead for testing > > w/o make install. > > > > Right now this is detecting some corruption induced by metadump/mdrestore > > with your full patchset in place, FWIW. > > > > Sorry I didn't send this sooner, kinda forgot I had it. really should turn it into an xfstest. > > Thanks! The corruption is caused by the last patch in the series. So > dev inodes can have attribute forks. I will have to zap the data and > the attr area separately. Ok, this is a better replacement for the fifth patch (copy/pasted into in-browser e-mail editor, might have broken formatting). Apparently the dev's inode contents are xfs_dev_t, and the rest of the data fork can be cleared. The attribute fork is handled later in process_inode. --- a/db/metadump.c +++ b/db/metadump.c @@ -2270,6 +2270,25 @@ process_inode_data( return 1; } +static int +process_dev_inode( + xfs_dinode_t *dip) +{ + if (XFS_DFORK_NEXTENTS(dip, XFS_DATA_FORK)) { + if (show_warnings) + print_warning("inode %llu has unexpected extents", + (unsigned long long)cur_ino); + return 0; + } else { + if (zero_stale_data) { + unsigned int size = sizeof(xfs_dev_t); + memset(XFS_DFORK_DPTR(dip) + size, 0, + XFS_DFORK_DSIZE(dip, mp) - size); + } + return 1; + } +} + /* * when we process the inode, we may change the data in the data and/or * attribute fork if they are in short form and we are obfuscating names. @@ -2322,7 +2341,10 @@ process_inode( case S_IFREG: success = process_inode_data(dip, TYP_DATA); break; - default: ; + default: + success = process_dev_inode(dip); + need_new_crc = 1; + break; } nametable_clear();
On Sun, Oct 28, 2018 at 3:36 PM Stefan Ring <stefanrin@gmail.com> wrote: > > Ok, this is a better replacement for the fifth patch (copy/pasted into > in-browser e-mail editor, might have broken formatting). I've also put it up at https://github.com/Ringdingcoder/xfsprogs-dev/commits/metadump-stale-data-v5-prelim for now. > Apparently the dev's inode contents are xfs_dev_t, and the rest of the > data fork can be cleared. The attribute fork is handled later in > process_inode. > > --- a/db/metadump.c > +++ b/db/metadump.c > @@ -2270,6 +2270,25 @@ process_inode_data( > return 1; > } > > +static int > +process_dev_inode( > + xfs_dinode_t *dip) > +{ > + if (XFS_DFORK_NEXTENTS(dip, XFS_DATA_FORK)) { > + if (show_warnings) > + print_warning("inode %llu has unexpected extents", > + (unsigned long long)cur_ino); > + return 0; > + } else { > + if (zero_stale_data) { > + unsigned int size = sizeof(xfs_dev_t); > + memset(XFS_DFORK_DPTR(dip) + size, 0, > + XFS_DFORK_DSIZE(dip, mp) - size); > + } > + return 1; > + } > +} > + > /* > * when we process the inode, we may change the data in the data and/or > * attribute fork if they are in short form and we are obfuscating names. > @@ -2322,7 +2341,10 @@ process_inode( > case S_IFREG: > success = process_inode_data(dip, TYP_DATA); > break; > - default: ; > + default: > + success = process_dev_inode(dip); > + need_new_crc = 1; > + break; > } > nametable_clear();
On 10/28/18 7:38 AM, Stefan Ring wrote: > On Sat, Oct 27, 2018 at 1:33 AM Eric Sandeen <sandeen@sandeen.net> wrote: >> >> Also, here's an old script I had lying around to test metadump. It's hacky, sorry. >> >> Things to watch out for... it looks for an "fsstress" binary from xfstests, so adjust that path. >> It freezes and unfreezes the test filesystem, if your mount fails it'll freeze >> the fs you're on. ;) There may be other rough spots. >> >> It also runs the xfs_metadump/xfs_db in your path; you could change that to a >> local ./xfs_metadump to run db/xfs_db from a git tree instead for testing >> w/o make install. >> >> Right now this is detecting some corruption induced by metadump/mdrestore >> with your full patchset in place, FWIW. >> >> Sorry I didn't send this sooner, kinda forgot I had it. really should turn it into an xfstest. > > Thanks! The corruption is caused by the last patch in the series. So > dev inodes can have attribute forks. I will have to zap the data and > the attr area separately. Cool, glad it was helpful. Check patch 2 as well, the "break;" is also causing corruption I think, and it should probably just be a no-op ";" It may get resolved by the time you get to patch5 but best to not have regressions along the way. Sorry for the piecemeal review here ;) -Eric
On Sun, Oct 28, 2018 at 6:13 PM Eric Sandeen <sandeen@sandeen.net> wrote: > Check patch 2 as well, the "break;" is also causing corruption I think, > and it should probably just be a no-op ";" Sorry about that. I think it's more than that. The single_fsb function takes care to only write something out when there is something to write and to only recalculate the crc when there has been a change. I'd like to carry over this spirit, but I have the impression that in the end the two functions would become so similar that they will just merge into one. > It may get resolved by the time you get to patch5 but best to not have > regressions along the way. Sorry for the piecemeal review here ;)
On 10/30/18 7:29 AM, Stefan Ring wrote: > On Sun, Oct 28, 2018 at 6:13 PM Eric Sandeen <sandeen@sandeen.net> wrote: >> Check patch 2 as well, the "break;" is also causing corruption I think, >> and it should probably just be a no-op ";" > > Sorry about that. I think it's more than that. The single_fsb function > takes care to only write something out when there is something to > write and to only recalculate the crc when there has been a change. > I'd like to carry over this spirit, but I have the impression that in > the end the two functions would become so similar that they will just > merge into one. I think the "recalculate crc" flagging is a little inconsistent, tbh - or maybe I should say it consistently recalculates it more often than it needs to? It probably makes sense to follow the existing examples of when it's set, and if there's any cleanup/optimization to do, that can be done later. -Eric