przemoc/metastore

Remove libattr dependency

kmdawson opened this issue · 13 comments

In building metastore (on Fedora 20) I found that the include line for xattr.h triggered a nosuch file or directory response. Looking in a number of man pages, and the book "The Linux Programming Interface", by Michael Kerrisk, I found an alternate location, which worked properly for me.

Additionally, I needed to add an include for errno.h.

Both of these files are installed by the glibc-headers-* packages.

I couldn't determine on what distros the location <attr/xattr.h> is correct.

Thanks for your attention.

/ken

The patch is:

diff --git a/metaentry.c b/metaentry.c
index b0ea69d..02e5bb8 100644
--- a/metaentry.c
+++ b/metaentry.c
@@ -25,13 +25,14 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <unistd.h>
-#include <attr/xattr.h>
+#include <sys/xattr.h>
 #include <limits.h>
 #include <dirent.h>
 #include <sys/mman.h>
 #include <utime.h>
 #include <fcntl.h>
 #include <stdint.h>
+#include <errno.h>

 #include "metastore.h"
 #include "metaentry.h"
diff --git a/metastore.c b/metastore.c
index de1bf07..0a49e3f 100644
--- a/metastore.c
+++ b/metastore.c
@@ -23,10 +23,11 @@
 #include <sys/stat.h>
 #include <getopt.h>
 #include <utime.h>
-#include <attr/xattr.h>
+#include <sys/xattr.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <errno.h>

 #include "metastore.h"
 #include "settings.h"

Thanks for reporting. I had to add fenced code block markings (three backticks) that you forgot yourself (without them the patch was completely unreadable). I'll take a closer look at this issue in upcoming days (and all the other metastore patches that are waiting in my queue to be finally reviewed and applied).

The patch works fine for me on Linux 3.18 with glibc 2.18 and uClibc 0.9.33.

Thanks for fixing that up; I'm new to Github protocol. Read up on GFM, so won't do that again.

@kmdawson Ideally you should:

  • fork the project on github,
  • clone it on your local computer,
  • create a branch dedicated for the fix, which could be named include-sys-xattr or remove-libattr-dependency for instance,
  • commit there your fix,
  • push it to the github,
  • create pull request in my repo on github.

Putting patches in body text of the issues is definitely not the correct way of submitting them, but I'll pretend it didn't happen this time. ;)

Would you mind sharing your full name and e-mail (possibly the one you use on github, unless you prefer some other one) for the purpose of committing your patch with proper authorship? If you'd like to avoid disclosing it in this issue directly, then you can mail me directly (you'll find address on my github profile), but it will land into repository in the end, so it will be public to anyone obtaining metastore sources.


I quickly looked at the matter. attr/xattr.h comes from libattr software. Installing this package headers (e.g. libattr1-dev in case of Debian) solves the problem, but it's indeed not necessary nowadays and by nowadays I mean last ~13 years in case of Linux, because sys/xattr.h apparently has been provided since glibc v2.3. David (and many others) most likely did the mistake due to outdated manual pages, that were fixed almost exactly one year ago by Micheal Kerrisk (his comment on kernel bugzilla). And attr/xattr.h includes errno.h, so it's only natural that it had to be added in the source files after this fix.

I don't use any other POSIX OS, so I cannot test #include <sys/xattr.h> working there myself right now (at least without much additional effort of setting several VMs - it may happen in the future, though).

@przmoc,
Here is the requested information:
Ken Dawson
dawsonwu@rahul.net
Thanks

@kmdawson Thanks. It's in impro/switch-xattr-header branch for now. I'm somehow reluctant to push it to master without checking it on non-Linux OSes.

@przemoc,
I'll see if I can get to a non-linux platform. I probably can get to Solaris, not so sure about BSD. Would that help?

@kmdawson To be brutally honest (I possibly shouldn't write it as a maintainer of unofficial metastore continuation, but I highly value transparency), I don't know the OSes metastore is supposed to work or where it simply works atm (I mean beside Linux). If possible I would prefer to preserve status quo in that regard (or make it even better, that goes without saying).

I am all for reducing dependencies, and taking into account that only libattr's header was used (there is no libattr's DSO dependency during linking), there shouldn't be any problem or unexpected breakage, but... Apparently there is more than one convention regarding these xattr syscalls and I just don't have time now to investigate it thoroughly, sorry.

Hopefully I'll prepare some VMs with different open source OSes for tests at some point and will dig in their headers/documentation (that of course doesn't require VM, but having VM for proper tests is still a must), but it won't be soon.

So if you can do some checks on your own, that would be surely helpful.

@przemoc: I discovered that I could get to an instance of FreeBSD, but have not yet found a Solaris platform. I investigated both, however, and discovered that it would need to be a port in both cases, since the APIs are similar, but have different naming.

I actually did a preliminary port to FreeBSD, while I was there, and have placed it in the FreeBSD branch on the fork I made of your metastore. It only contains the code changes, so is not ready to do a pull request, but I hoped you could take a look and see if it agrees with your design philosophy. I see that you are contemplating redoing the Makefile and source file layouts, so, likely this arrangement will have to change. If so, let me know how, and I can do it. This version builds and runs runs for me on both platforms, using just a vanilla permission-change scenario.

Thanks for your research, @kmdawson. I am quite busy lately, so I won't touch anything metastore-related (not even a glimpse) until upcoming weekend (or even later), sorry for that. Yet I still wanted to reply to let you know that real reply won't be prompt. ;)

@przemoc: no problem. Take your time.

This change (branch impro/switch-xattr-header) will be merged soon.

If any problem arises for someone, I expect she or he will report the bug and then we'll deal with broken compatibility. One thing is clear now at least, metastore wasn't supporting FreeBSD before, but it hopefully will in some version after v1.1.

Branch impro/switch-xattr-header has been merged.