Re: ffsv2 extattr support

["Thomas Schmitt" <scdbackup%gmx.net@localhost>]

Hi,

my development snapshot of xorriso is now able to record and
restore extattr on NetBSD.

Main work was to make it tolerate the error ENOENT that is
thrown by extattr_list_link(2) if the filesystem does not
support extattr.
Else it is much like FreeBSD. I will have to compare the
behavior of both in more detail.

I think getextattr(1) needs mending.
See at end of this mail: "getextattr(1) has problems"

-----------------------------------------------------------

One peculiarity shows up:

extattr_get_link(2) et.al. return the attribute content with
a trailing 0-byte.
This 0-byte seems to be a bug-or-feature of setextattr(1).

I overwrote an extattr in the ISO filesystem by xorriso means
without a trailing 0-byte. Then i extracted it into the FFSv1
filesystem and recorded it by xorriso again. No 0-byte appeared.
So the extattr_*(2) functions and xorriso are not the ones
who created those 0s.

Neither FreeBSD nor Linux show trailing 0-bytes with their
cleartext extattr/xattr.

I care for portability of attributes in namespace "user".
So it would be interesting to know whether the convention on
NetBSD is to have a 0-byte at the end of attribute content.
extattr(9) specifies "nul-terminated character string" for
names. But content is usually regarded as binary data, governed
by the length value and not by a terminating character.

If the 0-byte is convention, then i would consider to strip
it when recording and to add it when restoring.
But that would be feasible only if namespace "user" is reserved
for C character strings rather than byte arrays.


-----------------------------------------------------------
Demo with current development snapshot
  http://scdbackup.webframe.org/xorriso-1.3.7.tar.gz
  MD5 117954392e6359d5be8a2fcc0a965447
  Version timestamp :  2014.06.20.065402
  (in xorriso/xorriso_timestamp.h or in output of xorriso -version)
------------------------------------------------------------

# Create files with extattr in suitable filesystem /mnt/ffs1

  echo x > /mnt/ffs1/x
  echo y > /mnt/ffs1/y
  setextattr user bla xbla /mnt/ffs1/x
  setextattr user bla YYYbla /mnt/ffs1/y

  getextattr user bla /mnt/ffs1/x
  getextattr user bla /mnt/ffs1/y
# yields:
#   /mnt/ffs1/x     xbla
#   /mnt/ffs1/y     YYYbla

# Record in an ISO 9660 filesystem image (/tmp/extattr_test.iso),
# or in disk device (/dev/wd2f), or on optical medium (/dev/rcd0d).
# I avoid the directory /mnt/ffs1/.attributes which is not readable
# for the common user.

  drive=/tmp/extattr_test.iso

  xorriso \
    -for_backup \
    -outdev "$drive" \
    -blank as_needed \
    -add /mnt/ffs1/* --

# Inspect ISO

  xorriso \
    -for_backup \
    -indev "$drive" \
    -getfattr_r / --

# yields:
#   # file: mnt/ffs1/x
#   user.bla="xbla\000"
#   # file: mnt/ffs1/y
#   user.bla="YYYbla\000"

# Restore /mnt/ffs1 from ISO into a new directory /mnt/ffs1/xorriso_extracted

  xorriso  \
    -for_backup \
    -indev "$drive" \
    -osirrox on \
    -extract /mnt/ffs1 /mnt/ffs1/xorriso_extracted

# Inspect in FFSv1

  getextattr user bla /mnt/ffs1/xorriso_extracted/*

# yields:
#   /mnt/ffs1/xorriso_extracted/x   xbla
#   /mnt/ffs1/xorriso_extracted/y   YYYbla


------------------------------------------------------------
getextattr(1) has problems:
------------------------------------------------------------

  echo a > /mnt/ffs1/a
  setextattr user bla AAAAAAAAAAAAAAAA /mnt/ffs1/a

  getextattr user bla /mnt/ffs1/*
# yields:
#   /mnt/ffs1/a     AAAAAAAAAAAAAAAA
#   /mnt/ffs1/x     xblaAAAAAAAAAAA
#   getextattr: /mnt/ffs1/xorriso_extracted: failed: No message available
# Note that the content of ./x is shown mangled and ./y is not listed.

  getextattr user bla /mnt/ffs1/x
# yields:
#   /mnt/ffs1/x     xbla

  getextattr user bla /mnt/ffs1/y
# yields:
#   /mnt/ffs1/y     YYYbla

# Note that all three files were treated with setextattr(1)
# and not created by xorriso. I am not to blame (TM).

------------------------------------------------------------

The problem with the mangled values is in
  /usr/src/usr.bin/extattr/extattr.c

In line 370 it asks for the size of the attribute

                        if (flag_nofollow)
                                error = extattr_get_link(argv[arg_counter],
                                    attrnamespace, attrname, NULL, 0);
                        else
                                error = extattr_get_file(argv[arg_counter],
                                    attrnamespace, attrname, NULL, 0);
                        if (error < 0)
                                break;

submits it to the local function

                        mkbuf(&buf, &buflen, error);

which leaves buflen unchanged if the new demand "error" is smaller
than the previous one. 

In line 409 it uses buflen as the length of the value

                                fwrite(buf, buflen, 1, stdout);

The small patch would be

-                               fwrite(buf, buflen, 1, stdout);
+                               fwrite(buf, error, 1, stdout);

But actually i think one should put the length into "val_len"
and not use "error" in non-error situations.
(I suspect the reason for this bug is that "buflen" is too luring
 to the tired programmer if only "error" is the alternative.)

------------------------------------------------------------

The problem with the early abort after the first error is in
line 376:

                        if (error < 0)
                                break;

I think that a warning and "continue" is the more appropriate
reaction. (The "break" is for "switch", not for "for".)

                        if (error < 0) {
                                warn("%s: failed", argv[arg_counter]);
                                continue;
                        }

------------------------------------------------------------

After changing what i deem to be needy, i get

  $ ./getextattr user bla /mnt/ffs1/*
  /mnt/ffs1/a     AAAAAAAAAAAAAAAA
  /mnt/ffs1/x     xbla
  getextattr: /mnt/ffs1/xorriso_extracted: failed: No message available
  /mnt/ffs1/y     YYYbla

------------------------------------------------------------

What advise do you have for a PR and patch ?

Shall i try to keep the change minimal, or shall i remove the
inappropriate use of variable "error" in favor of "val_len" ?


Have a nice day :)

Thomas