HFS+ compression support [was Re: Backing up resource forks]

Mike Bombich mike at bombich.com
Wed Mar 16 12:52:19 MDT 2011


Hi Wayne:

	Thanks for taking a look at that patch. A few comments inline below...

On Mar 14, 2011, at 12:42 AM, Wayne Davison wrote:

> On Fri, Mar 11, 2011 at 8:05 AM, Mike Bombich <mike at bombich.com> wrote:
> FWIW, I updated http://www.bombich.com/rsync.html just recently with a minor bug fix to my HFS+ compression patch (preserving resource forks when --fileflags was not used).
> 
> I looked over your patch to see about getting it adapted for 3.0.8.  While doing that I made some changes and fixes for various things.  Some notes on your diff:
> Firstly, I made use of some of the fixes in your patch for the fileflags and crtimes patches -- thanks!
I have a couple more, too, especially around the --force-change functionality.  I'll file a separate bug for that though.

> Your patch set an ITEM_REPORT_XATTR flag in a FLAG_* variable, which really set FLAG_HLINK_DONE, which is potentially harmful for the code's ability to retrieve values for a file's data.
I caught that one too, but failed to make the change in my posted patch. Mea culpa!


>  While it would have been possible to change this to a new flag, I instead changed the xattr idiom to pass the compressed-data xattr as a normal abbreviated entry, but with the mtime in the checksum.  This lets the code work normally to figure out if the xattr field is identical or not (and incorporates the size into the check, for added safety).
I like this a lot, thanks!

> I got rid of the compat_flag changes in compat.c, since they are not needed and are potentially harmful for forward compatibility (if I should ever use that compatibility bit in a future release).  Instead, the code in options.c dies if the option is not available.

But having the option (or not) on the receiving end isn't comparable to the filesystem having support for the feature.  Suppose you have two 10.6 systems and each has a build of rsync that includes HFS compression support. On the sending side, the source is set to an HFS+ volume with compressed files. On the receiving side, the destination is set to a FAT32 volume. In this case, with either --protect-decmpfs or, more appropriately, the --hfs-compression option, the receiver disables hfs-compression but the sender does not -- it has no idea that the receiver's fs doesn't support it. Subsequently, the sender masks the uncompressed data fork and sends the decmpfs xattrs instead. Worse, the 0x20 flag isn't supported on FAT32, so even if the xattrs were successfully written, the UF_COMPRESSED flag isn't set and the file is essentially corrupted.

The receiver should probably just exit with RERR_UNSUPPORTED if the --protect-decmpfs option is used and the destination filesystem doesn't support it.  I was hoping to provide failover to decompression for the --hfs-compression option, though, which is what the use of the compat flags offers.

> The code for checking the destination file system's compression support was moved to main.c (since that is what calls it).
> The repeating code that tweaks the stat-struct was moved into x_stat(), so it's in just one spot.

There's still one in recv_generator(). I'm not familiar enough with the fuzzy option to know whether this is necessary, but I would assume that it is not because every file entry in flist should have been "pretreated" based on the preserve_hfs_compression value when x_stat was run.  Also, this same block of code should be present in x_lstat and x_fstat.

> I fixed some issues that caused the "make check" tests to fail.
> I changed your rsync.1 change into an rsync.yo change (since the manpages are generated from the yodl files).
> I made some style changes, getting rid of trailing whitespace, using /* ... */ for comments, etc.
> I checked in the result into the patches git repo as hfs-compression.diff.
You made a comment in hlink.c:


> 	if (hard_link_one(file, fname, oldname, 0)) {
> 		/* If the hard link was successful, restat the destination file */
> 		/* XXX is this really needed? */
> 		link_stat(fname, &sxp->st, 0);
> 		if (itemizing) {
> 			itemize(fname, file, ndx, statret, sxp,
> 				ITEM_LOCAL_CHANGE | ITEM_XNAME_FOLLOWS, 0,
> 				realname);
> 		}
> 		if (code != FNONE && verbose)
> 			rprintf(code, "%s => %s\n", fname, realname);
> 		return 0;
> 	}

My original patch was actually:

> 		statret = link_stat(fname, &sxp->st, 0);

What I found was that hard linked, compressed files were getting corrupted. The file that got copied over first was fine, and the hard linked file was fine immediately after the hard link was created, but set_file_attrs was clearing the UF_COMPRESSED flag.

I'm not clear why we call itemizing() here, I would assume that calling it once on the original hard link is adequate. Because the new hard link's mtime hasn't been set yet, xattr_diff for a hard link with a decmpfs xattr will always indicate that the xattrs differ from the original file on the source. Further, we're passing statret here -- why would  we pass the result of link_stat *before* the hard link was created?  If we set statret to the result of the repeated link_stat above, itemize then respects the current sxp for the new hard link. This is still, unfortunately, inadequate, unless the mtime of the new hard link is updated on disk before the link_stat call. Lastly, the hard linked file will be *identical* to the source link, therefore we shouldn't need to apply any other changes to this file that are or will be applied to the original hard link.

My tests showed great results by removing the itemize() call altogether. The updated patch (http://home.bombich.com/hfs-compression_bombich.diff) includes that change and my comments above.  

The only outstanding issue is the inability to disable the --hfs-compression option on the sending side if the receiver's filesystem doesn't support it. If you can think of a way to do that without the compat_flags, I'm happy to put it through some more testing.

Mike


> If you would test the result, I'd appreciate it.  I verified that it compiles and passes the testsuite, but I don't currently have any compressed files to test it against.
> 
> To snag the code, either grab things from git or download a tar file of the resulting code.  The git method:
> 
> git clone git://git.samba.org/rsync
> git clone git://git.samba.org/rsync-patches rsync/patches
> cd rsync
> git checkout b3.0.x
> cd patches
> git checkout b3.0.x
> cd ..
> 
> Then just follow the directions in the patches/hfs-compression.diff for applying the 3 patches and compiling.
> 
> Or you can snag this tar file which has all the patches applied:
> 
> http://opencoder.net/rsync-hfs-compression.tar.gz
> 
> ..wayne..

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.samba.org/pipermail/rsync/attachments/20110316/eff588d0/attachment.html>


More information about the rsync mailing list