[Xen-devel] [PATCH 0/2] Remove locking.sh dependency on perl

[Xen-devel] [PATCH 0/2] Remove locking.sh dependency on perl

[Jason Andryuk]

Perl is a large dependency for locking.sh's single use of comparing a
file descriptor's and a file's inodes.  Many systems don't otherwise
require perl, so dropping this use eliminates the dependency.

Replace the open-coded perl with an equivalent C implementation.

Jason Andryuk (2):
  tools/helpers: Introduce cmp-fd-file-inode utility
  Linux/locking.sh: Use cmp-fd-file-inode for lock check

 .gitignore                        |  1 +
 tools/helpers/Makefile            |  3 +++
 tools/helpers/cmp-fd-file-inode.c | 43 +++++++++++++++++++++++++++++++
 tools/hotplug/Linux/locking.sh    | 10 ++-----
 4 files changed, 49 insertions(+), 8 deletions(-)
 create mode 100644 tools/helpers/cmp-fd-file-inode.c

-- 
2.24.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/2] tools/helpers: Introduce cmp-fd-file-inode utility

[Jason Andryuk]

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

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;
+}
-- 
2.24.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/2] Linux/locking.sh: Use cmp-fd-file-inode for lock check

[Jason Andryuk]

Replace perl with cmp-fd-file-inode when checking that the lock file
descriptor and lockfile inodes match.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 tools/hotplug/Linux/locking.sh | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/tools/hotplug/Linux/locking.sh b/tools/hotplug/Linux/locking.sh
index c6a7e96ff9..de468c4bb5 100644
--- a/tools/hotplug/Linux/locking.sh
+++ b/tools/hotplug/Linux/locking.sh
@@ -50,14 +50,8 @@ claim_lock()
         # 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.
-        rightfile=$( perl -e '
-            open STDIN, "<&'$_lockfd'" or die $!;
-            my $fd_inum = (stat STDIN)[1]; die $! unless defined $fd_inum;
-            my $file_inum = (stat $ARGV[0])[1];
-            print "y\n" if $fd_inum eq $file_inum;
-                             ' "$_lockfile" )
-        if [ x$rightfile = xy ]; then break; fi
+        # file system to find the inum.
+        if cmp-fd-file-inode $_lockfd $_lockfile ; then break; fi
 	# Some versions of bash appear to be buggy if the same
 	# $_lockfile is opened repeatedly. Close the current fd here.
         eval "exec $_lockfd<&-"
-- 
2.24.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] tools/helpers: Introduce cmp-fd-file-inode utility

[Ian Jackson]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] tools/helpers: Introduce cmp-fd-file-inode utility

[Jason Andryuk]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] tools/helpers: Introduce cmp-fd-file-inode utility

[George Dunlap]

[-- Attachment #1.1: Type: text/plain, Size: 1345 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 2024 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] tools/helpers: Introduce cmp-fd-file-inode utility

[Ian Jackson]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] tools/helpers: Introduce cmp-fd-file-inode utility

[Jason Andryuk]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel