| Message ID | 20200226152013.12200-2-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove locking.sh dependency on perl | expand |
Jason Andryuk writes ("[PATCH 1/2] tools/helpers: Introduce cmp-fd-file-inode utility"):
> This is a C implementation of the perl code inside of locking.sh to
> check that the locked file descriptor and lock file share the same inode
> and therefore match. One change from the perl version is replacing
> printing "y" on success with exit values of 0 (shell True) and 1 (shell
> False).
Maybe it would be better to use stat(1) ? On Linux
stat -L -c%D.%i /dev/stdin blah.lock
or some such, and then compare the two numbers.
I'm reluctant to host a general-purpose shell utility in xen.git, no
matter how useful...
Thanks,
Ian.
On Wed, Feb 26, 2020 at 10:48 AM Ian Jackson <ian.jackson@citrix.com> wrote: > > Jason Andryuk writes ("[PATCH 1/2] tools/helpers: Introduce cmp-fd-file-inode utility"): > > This is a C implementation of the perl code inside of locking.sh to > > check that the locked file descriptor and lock file share the same inode > > and therefore match. One change from the perl version is replacing > > printing "y" on success with exit values of 0 (shell True) and 1 (shell > > False). > > Maybe it would be better to use stat(1) ? On Linux > stat -L -c%D.%i /dev/stdin blah.lock > or some such, and then compare the two numbers. > > I'm reluctant to host a general-purpose shell utility in xen.git, no > matter how useful... Thanks for taking a look. I'd be happy to use stat if it works. The comment in locking.sh above the usage is: # We can't just stat /dev/stdin or /proc/self/fd/$_lockfd or # use bash's test -ef because those all go through what is # actually a synthetic symlink in /proc and we aren't # guaranteed that our stat(2) won't lose the race with an # rm(1) between reading the synthetic link and traversing the # file system to find the inum. Perl is very fast so use that. ...which I thought ruled out stat. Regards, Jason
On Wed, Feb 26, 2020 at 3:49 PM Ian Jackson <ian.jackson@citrix.com> wrote: > Jason Andryuk writes ("[PATCH 1/2] tools/helpers: Introduce > cmp-fd-file-inode utility"): > > This is a C implementation of the perl code inside of locking.sh to > > check that the locked file descriptor and lock file share the same inode > > and therefore match. One change from the perl version is replacing > > printing "y" on success with exit values of 0 (shell True) and 1 (shell > > False). > > Maybe it would be better to use stat(1) ? On Linux > stat -L -c%D.%i /dev/stdin blah.lock > or some such, and then compare the two numbers. > > I'm reluctant to host a general-purpose shell utility in xen.git, no > matter how useful... > Do you have any other suggestions? I agree it's not great to have loads of little helper programs lying around. But it's a lot better than pulling in a full perl installation for a single line. I sort of feel like part of the issue is that this is written in shell at all. The necessity to fall back to perl seems to me to indicate that bash is the wrong language for what needs to happen here. If locking.sh were locking.c instead, this entire series probably wouldn't be necessary. If no better options are forthcoming, I think we should accept something like this until something better comes along. -George
Jason Andryuk writes ("Re: [PATCH 1/2] tools/helpers: Introduce cmp-fd-file-inode utility"):
> I'd be happy to use stat if it works. The comment in locking.sh above
> the usage is:
> # We can't just stat /dev/stdin or /proc/self/fd/$_lockfd or
> # use bash's test -ef because those all go through what is
> # actually a synthetic symlink in /proc and we aren't
> # guaranteed that our stat(2) won't lose the race with an
> # rm(1) between reading the synthetic link and traversing the
> # file system to find the inum. Perl is very fast so use that.
>
> ...which I thought ruled out stat.
Well read.
I have done some more testing and in my tests (on Debian stretch)
stat -L - <some-file
does this
fstat64(0, {st_mode=S_IFREG|0664, st_size=117844, ...}) = 0
(according to strace) which is precisely what is needed.
Oddly, it also does this
fstat64(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 171), ...}) = 0
but it doesn't seem to do anything with the results, so I think
that's harmless.
I wrote that comment in 2012. Presumably `stat -L -' has appeared in
the meantime.
The synthetic symlink may be a red herring anyway; nowadays at least,
I am told by someone who read the Linux kernel source that
the name comes from the `readlink' method on the link inode, but a
different method entirely -- `get_link' -- is used by `namei' to
actually resolve the link to a destination inode.
But using `-' is clearly fine, like this I think:
mariner:~> stat -c%D.%i -L - t <t
fe04.844307
fe04.844307
mariner:~>
Sorry to muddy the waters.
Ian.
On Thu, Mar 5, 2020 at 2:12 PM Ian Jackson <ian.jackson@citrix.com> wrote: > > Jason Andryuk writes ("Re: [PATCH 1/2] tools/helpers: Introduce cmp-fd-file-inode utility"): > > I'd be happy to use stat if it works. The comment in locking.sh above > > the usage is: > > # We can't just stat /dev/stdin or /proc/self/fd/$_lockfd or > > # use bash's test -ef because those all go through what is > > # actually a synthetic symlink in /proc and we aren't > > # guaranteed that our stat(2) won't lose the race with an > > # rm(1) between reading the synthetic link and traversing the > > # file system to find the inum. Perl is very fast so use that. > > > > ...which I thought ruled out stat. > > Well read. > > I have done some more testing and in my tests (on Debian stretch) > stat -L - <some-file > does this > fstat64(0, {st_mode=S_IFREG|0664, st_size=117844, ...}) = 0 > (according to strace) which is precisely what is needed. > > Oddly, it also does this > fstat64(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 171), ...}) = 0 > but it doesn't seem to do anything with the results, so I think > that's harmless. I think this stat is from glibc before printing. `stat file -` re-orders the fstat(1) before fstat(0). > I wrote that comment in 2012. Presumably `stat -L -' has appeared in > the meantime. I peaked at coreutils git, and - was added in 2009. But I had no idea of the magic '-'. Thank you for finding it. > The synthetic symlink may be a red herring anyway; nowadays at least, > I am told by someone who read the Linux kernel source that > the name comes from the `readlink' method on the link inode, but a > different method entirely -- `get_link' -- is used by `namei' to > actually resolve the link to a destination inode. > > But using `-' is clearly fine, like this I think: > > mariner:~> stat -c%D.%i -L - t <t > fe04.844307 > fe04.844307 > mariner:~> > > Sorry to muddy the waters. Thanks again for finding the solution. Regards, Jason
diff --git a/.gitignore b/.gitignore index 4ca679ddbc..897f878eef 100644 --- a/.gitignore +++ b/.gitignore @@ -164,6 +164,7 @@ tools/fuzz/x86_instruction_emulator/x86-emulate.[ch] tools/helpers/_paths.h tools/helpers/init-xenstore-domain tools/helpers/xen-init-dom0 +tools/helpers/cmp-fd-file-inode tools/hotplug/common/hotplugpath.sh tools/hotplug/FreeBSD/rc.d/xencommons tools/hotplug/FreeBSD/rc.d/xendriverdomain diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile index f759528322..7daf5c46ca 100644 --- a/tools/helpers/Makefile +++ b/tools/helpers/Makefile @@ -8,6 +8,7 @@ include $(XEN_ROOT)/tools/Rules.mk PROGS += xen-init-dom0 ifeq ($(CONFIG_Linux),y) PROGS += init-xenstore-domain +PROGS += cmp-fd-file-inode endif XEN_INIT_DOM0_OBJS = xen-init-dom0.o init-dom-json.o @@ -40,12 +41,14 @@ install: all $(INSTALL_PROG) xen-init-dom0 $(DESTDIR)$(LIBEXEC_BIN) ifeq ($(CONFIG_Linux),y) $(INSTALL_PROG) init-xenstore-domain $(DESTDIR)$(LIBEXEC_BIN) + $(INSTALL_PROG) cmp-fd-file-inode $(DESTDIR)$(LIBEXEC_BIN) endif .PHONY: uninstall uninstall: ifeq ($(CONFIG_Linux),y) rm -f $(DESTDIR)$(LIBEXEC_BIN)/init-xenstore-domain + rm -f $(DESTDIR)$(LIBEXEC_BIN)/cmp-fd-file-inode endif rm -f $(DESTDIR)$(LIBEXEC_BIN)/xen-init-dom0 diff --git a/tools/helpers/cmp-fd-file-inode.c b/tools/helpers/cmp-fd-file-inode.c new file mode 100644 index 0000000000..886ea888ed --- /dev/null +++ b/tools/helpers/cmp-fd-file-inode.c @@ -0,0 +1,43 @@ +#include <stdio.h> +#include <stdlib.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> + +void usage(const char * prog) +{ + fprintf(stderr, +"%s <fd> <filename>\n" +"Checks that the open file descriptor (referenced by number) has the same\n" +"inode as the filename.\n" +"Returns 0 on match and 1 on non-match\n", prog); +} + +int main(int argc, char *argv[]) +{ + struct stat fd_statbuf, file_statbuf; + int ret; + int fd; + + if (argc < 3) { + usage(argv[0]); + return 1; + } + + fd = strtoul(argv[1], NULL, 0); + + ret = fstat(fd, &fd_statbuf); + if (ret) { + return 1; + } + + ret = stat(argv[2], &file_statbuf); + if (ret) { + return 1; + } + + if (fd_statbuf.st_ino == file_statbuf.st_ino) + return 0; + else + return 1; +}
This is a C implementation of the perl code inside of locking.sh to check that the locked file descriptor and lock file share the same inode and therefore match. One change from the perl version is replacing printing "y" on success with exit values of 0 (shell True) and 1 (shell False). Requiring perl is a large dependency for the single use, so a dedicated utility removes that dependency for systems that otherwise would not install perl. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- .gitignore | 1 + tools/helpers/Makefile | 3 +++ tools/helpers/cmp-fd-file-inode.c | 43 +++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+) create mode 100644 tools/helpers/cmp-fd-file-inode.c