Created attachment 4550 [details]
Patch to add support for solaris extended attributes

I had some preliminary Solaris xattr code in the git repo, though it had some bugs.  I used some ideas from your patch to get squash the ones I saw, but the result is not tested.  If you could snag the latest git repo or the latest "nightly" tar file and try it out, let me know if you find any problems.

Note that my version just ignores symlink xattrs, as I'm not liking the idea of attaching symlink xattrs to the parent directory.

I have been thinking that --fake-super should convert symlinks into regular files on a system where NO_SYMLINK_XATTRS is defined (saving the symlink either in the file contents or as a special rsync xattr), which would at least allow backups of symlink xattrs onto a system that doesn't support that.

I thought of saving the symlink contents as normal file contents as well, but I wanted to keep my changes out of the core rsync code. Attaching the symlink attributes to the parent directory was carefully written to work for rsync and there are problems with my code that prevent it being more general.

I'll try the nightly tar. I'm pretty sure that my corporate firewalls are git hostile. I'll write some code to handle NO_SYMLINK_XATTRS in the next few days because it's a show stopper for me.

I tried this patch which of course is much cleanear of the one I sent to the devel mailing list, but this does not work for me. I am only testing from linux to opensolaris using --fake-super

The rsync.%stat xattr is written correctly but i always get errors like these in reading:


rsync: failed to read xattr rsync.%stat for "/data/cavalli/test/.": Error 0 (0)
./

Still not sure where the problem is (

I think the problem is that bufpos (and so ret) is always set to "-1" in this code ( in read_xattr).

If cnt==0 shouldn't this indicate that end of file is reached?

 for (bufpos = 0; bufpos < buflen; ) {
                        ssize_t cnt = read(attrfd, buf + bufpos, buflen - bufpos);
                        if (cnt <= 0) {
                                if (cnt < 0 && errno == EINTR)
                                        continue;
                                 bufpos = -1; 
                                break;
                        }
                        bufpos += cnt;
                }
                ret = bufpos;

Make sure you're trying the latest version from the git repo, a nightly tar file, or a file grabbed from the unpacked files:

    http://rsync.samba.org/download.html

The code you cited fails on read() == 0 because it only tries to read data when the data isn't long enough yet, and an early EOF should be an error, AFAIK.

I do not undestand you point. The last niglty snapshot and also the last git commit show the same results. Actually I put this line in the code to do some debugging:

 ssize_t cnt = read(attrfd, buf + bufpos, buflen - bufpos);
 rsyserr(FERROR_XFER, errno, "read %d bytes, buflen %d",cnt,buflen);


This clearly shows two reads 

rsync: read 13 bytes, buflen 255: Error 0 (0)
rsync: read 0 bytes, buflen 255: Error 0 (0)
rsync: failed to read xattr rsync.%stat for "/data/cavalli/test/.": Error 0 (0)

0 on the second read indicates EOF reached but not early EOF in this case I think: what is wrong with this reasoning?

Aha.  The loop is being limited by buflen instead of sb.st_size.  Change this:

-               for (bufpos = 0; bufpos < buflen; ) {
-                       ssize_t cnt = read(attrfd, buf + bufpos, buflen - bufpos);
+               for (bufpos = 0; bufpos < sb.st_size; ) {
+                       ssize_t cnt = read(attrfd, buf + bufpos, sb.st_size - bufpos);

Thanks for the correction in comment #8. I have another small change to suggest:

-       if ((attrfd = attropen(path, name, O_CREAT|O_WRONLY|O_NOFOLLOW, mode)) < 0)
+       if ((attrfd = attropen(path, name, O_CREAT|O_TRUNC|O_WRONLY|O_NOFOLLOW, mode)) < 0)


O_TRUNC is needed otherwise for instance if you change a file owner from 1002 to 0 you start with attribute set to

100640 0,0 1002:1002

and end up with this mess:

100640 0,0 0:02:1002

Also about O_NOFOLLOW: does it should avoid setting attributes from symlinks? From the open(2) manpage 
"   If the path names a symbolic link, open() fails and sets
         errno to ELOOP"

But unfortunately this does not work with attropen. There is also  AT_SYMLINK_NOFOLLOW but I do not exatly understand its job with fstatat...

The only way I came up was a stat of path...

Finally, what about opening a discussion about where/how to write attributes of symlinks?

Thanks for the write fix.

The latest code turns a symlink into a file when using --fake-super on a system that has NO_SYMLINK_XATTRS defined in config.h.  You can snag that via git or the latest nightly tar file.

(In reply to comment #10)
> Thanks for the write fix.
> 
> The latest code turns a symlink into a file when using --fake-super on a system
> that has NO_SYMLINK_XATTRS defined in config.h.  You can snag that via git or
> the latest nightly tar file.
> 

Thanks for the immediate action, it seems to work as expected, both ways during backup and restore. Actually now, after some reasoning, I think your solution of using a file is much cleaner than putting the xattr in the parent dir.

Now I only see an issue in "file browsing". Suppose I want to browse a backup: I do

rsync -a --rsync-path='... --fake-super' user@host:/backup_dir/

and I see this

lrwxrwxrwx          10 2009/09/03 10:56:44 link-a-dir -> directory/
drwxr-xr-x           3 2009/09/03 10:56:50 directory

So let's cd to "link-a-dir"  I get:

rsync -a --rsync-path='... --fake-super' user@host:/backup_dir/link-a-dir/
 
rsync: change_dir "/data/cavalli/test/link-a-dir" failed: Not a directory (20)

Actually this is not exactly a real issue, but just a usability improvement... I don't know if I clearly explained my concern.

Probably this could be even done outside of rsync itself, writing something like described in rsyncsh.txt...

Small correction to sys_lremovexattr, NOT UNDERSTOOD, unlink of attributes works only of attropen is called without O_RDWR .... 

This is not strictly related with fake-super on receiving side, but with -X on sender size.

@@ -217,7 +217,7 @@ int sys_lremovexattr(const char *path, const char *name)
        int attrdirfd;
        int ret;
 
-       if ((attrdirfd = attropen(path, ".", O_RDWR)) < 0)
+       if ((attrdirfd = attropen(path, ".", O_RDONLY)) < 0)
                return -1;
 
        ret = unlinkat(attrdirfd, name, 0);


During my tests this was the error I got 

rsync: rsync_xal_clear: lremovexattr("cavalli","pippo") failed: Is a directory (21)


user.pippo was the user extended attribute on linux side correctly backed up by -X, and now I was trying to delete it.