Follow-up Comment #7, patch #9754 (group findutils): I expect you're right. I'm no expert in find's code structure. But I'd suggest that rather than HAVE_INODE_FLAGS (which presupposes an implementation), a name like HAVE_ATTRIBUTES or HAVE_FLAGS might be better, because it's higher level (and then each implementation could have its own define such as HAVE_LINUX_FS_H or HAVE_ATTRIBUTES_LINUX). For example, Solaris's file attributes are stored in the extended attributes, not the inode. But maybe the name is fine.
However: The attr_to_iflag (and attr_to_rate and get_iflags) functions are
guarded by #ifdef HAVE_INODE_FLAGS. I don't think that's enough. Another
system might have HAVE_INODE_FLAGS, but a different implementation, and that
function refers to symbols like FS_APPEND_FL without being guarded by #ifdef
HAVE_LINUX_FS_H. Either the whole function should be guarded by
HAVE_LINUX_FS_H, and each implementation has its own entire attr_to_iflag
function, or the function is only guarded by HAVE_INODE_FLAGS, and inside it,
each system's implementation is guarded by its separate define (like
HAVE_LINUX_FS_H). Maybe that can wait until the second implementation is
added, but setting the scene now wouldn't be a bad idea. Maybe the find
developers would have a preference.
By the way, in 2019 you indicated that linux/fs.h applied to multiple
filesystem types but ext2-style attributes were specific to the ext2 family of
filesystems. That doesn't seem correct. The seemingly ext2-specific libe2p
interface (fgetflags) from e2fsprogs also applies to multiple filesystem
types. I hadn't tested it on xfs but it works at least on ext4 and btrfs. I've
just tested it on xfs, and it works there too. I don't know which is the
better interface, but berny did mention some flags were missing from the
patch. They are available via the fgetflags() interface. So maybe it would be
better to switch to using fgetflags()? [No, see below]
Warning: If you did change to use fgetflags() note that there's currently a
bug when it returns -1. If it did so because ioctl() failed, then it sets the
flags to an uninitialised value, so the flags need to be set to zero when
fgetflags fails. I've submitted a patch so maybe it'll get fixed.
Actually, I just checked linux/fs.h and it does have CaseFold and Verity and
others that aren't in the patch. Presumably they have been added since 2019.
Comparing ext2fs/ext2_fs.h and libux/fs.h, there are very few differences.
Only linux/fs.h has EOFBLOCKS (ext2fs/ext2_fs.h had it but removed it). Only
ext2fs/ext2_fs.h has SNAPFILE, SNAPFILE_DELETED, SNAPFILE_SHRUNK. This is
what's on a debian12 system so it's old. There may be new attributes by now
(No it's the same on github). So it sounds like the only advantage to using
fgetflags instead would be to search for snapshot inodes. [No, see the end]
Anyway, the patch is missing FS_DIRTY_FL, FS_COMPRBLK_FL, FS_NOCOMP_FL,
FS_BTREE_FL, FS_IMAGIC_FL, FS_HUGE_FILE_FL, FS_VERITY_FL, FS_EA_INODE_FL,
FS_EOFBLOCKS_FL (maybe), FS_DAX_FL, and FS_CASEFOLD_FL. It might as well
include them.
Also, I think it's a good idea at least add chattr(1) to the SEE ALSO section
in find.1 (and the info equivalent). Although I think it would be better to
list the attribute letters explicitly in find's documentation, the way it
includes some strftime format conversions, rather than only directing the
reader to look at strftime's own manpage. It makes it easier for the reader
because they don't have to look up another manpage. But find's developers
might disagree. Adding a list of letters and explanations for each
implementation might become unwieldy with more implementations (but I'd still
recommend it). Although, since the chattr manpage doesn't mention all of the
attributes, documenting them in find's manpage is necessary. chattr(1) (on
debian12) only mentions 22 attributes, but linux/fs.h has 29 and
ext2fs/ext2_fs.h has 31. So there are extra ones that need to be documented
(if find should support all of them even though chattr doesn't).
It looks like fgetflags and this patch both use ioctl but with a different
command: EXT2_IOC_GETFLAGS or FS_IOC_GETFLAGS. But looking deeper, both are
defined to the exact same thing. So there is no difference, just what gets
listed in headers. It's the same ioctl. So snapshots would work via
FS_IOC_GETFLAGS, it just isn't mentioned in linux/fs.h. So the only advantage
to fgetflags() is that it's ported to other platforms (but it might not get
installed often on other platforms).
If all of the attributes are to be supported, letters would need to be chosen
for the 9 that are not mentioned in chattr(1). Here they are along with my
suggestions for letters to go with them:
y=dirty (0x00000100)
b=comprblk (0x00000200)
B=btree (0x00001000)
M=imagic (0x00002000)
h=hugefile (0x00040000)
n=ea_inode (0x00200000)
z=snapfile (0x01000000)
Z=snapfile_deleted (0x04000000)
U=snapfile_shrunk (0x08000000)
Of course, it begs the question: Why doesn't chattr mention these? Maybe that
doesn't matter. Presumably they are usable via fsetflags() or
ioctl(EXT2_IOC_SETFLAGS) or ioctl(FS_IOC_SETFLAGS).
_______________________________________________________
Reply to this item at:
<https://savannah.gnu.org/patch/?9754>
_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/
signature.asc
Description: PGP signature
