Re: [Xen-devel] [PATCH v2] tools/xenstore: Do not abort xenstore-ls if a node disappears while iterating

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

On Wed, 2019-08-07 at 16:37 +0100, Ian Jackson wrote:
> I think it is not safe to continue if we get a permissions error.  I
> think that would mean "we were not able to determine whether this node
> exists", not "this node does not exist".

Either way, I interpreted it more as "haha screw you, now I'm not going
to tell you whether any *other* node exists".

This is the test case I actually started with:

(dom0) # for a in `seq 1 1000` ; do xenstore-write /local/domain/2/foo/$a $a ; 
done

Now simultaneously:

(dom0) # for a in `seq 1 999` ; do xenstore-rm /local/domain/2/foo/$a  ; done
(dom2) # while true ; do ./xenstore-ls -p /local/domain/2/foo | grep -c 1000;  
done

I do expect to see node 1000, every time. With my patch, that works.

> So with a permissions error silently ignored, xenstore-ls might
> silently produce partial output.

Even without the race condition, we get partial output. In this test
case, observe that node four is absent from what dom2 reports, and we
get *two* error messages about node three.

(dom0) # xenstore-ls -p /local/domain/2/foo
one = "one"  . . . . . . . . . . . . . . . . . . . . . . . .  (n0,r2)
two = "two"  . . . . . . . . . . . . . . . . . . . . . . . .  (n0,r2)
three = "three"  . . . . . . . . . . . . . . . . . . . . . .  (n0)
four = "four"  . . . . . . . . . . . . . . . . . . . . . . .  (n0,r2)

(dom2) # xenstore-ls -p /local/domain/2/foo
one = "one"  . . . . . . . . . . . . . . . . . . . . . . . .  (n0,r2)
two = "two"  . . . . . . . . . . . . . . . . . . . . . . . .  (n0,r2)
three:
xenstore-ls: 
could not access permissions for three: Permission denied

xenstore-ls: xs_directory (/local/domain/2/foo/three): Permission denied



The tool actually makes three calls for each node it touches. If the
xs_read() fails, it silently prints ":\n" and doesn't report the errno.

If the (optional) xs_get_permissions() fails, it *warns* but continues,
attempting to recurse into the node in question.

If the xs_directory() fails, it aborts and doesn't even continue.

My v2 patch makes the third case (xs_directory()) behave like the first
(xs_read()), and silently continue. It gives me:

(dom2) # ./xenstore-ls -p  /local/domain/2/foo
one = "one"  . . . . . . . . . . . . . . . . . . . . . . . .  (n0,r2)
two = "two"  . . . . . . . . . . . . . . . . . . . . . . . .  (n0,r2)
three:
xenstore-ls: 
could not access permissions for three: Permission denied

four = "four"  . . . . . . . . . . . . . . . . . . . . . . .  (n0,r2)


(dom2) # ./xenstore-ls /local/domain/2/foo
one = "one"
two = "two"
three:

four = "four"

> Given that the code doesn't have a way to print an error message and
> record the error code or exit status for later, I think the best
> approach is probably exactly David's patch only without the mention of
> EACCES.

Well... here's an incremental straw man patch based on the existing v2,
which will print an error for the *first* operation that fails, and
should cope with race conditions too.

Tested with

(dom0) # while true; do xenstore-chmod /local/domain/2/foo/three n0; 
xenstore-chmod /local/domain/2/foo/three n0 r2 ; done
(dom2) # while true ;  do echo ================= ; sudo ./xenstore-ls  -p  
/local/domain/2/foo; done

diff --git a/tools/xenstore/xenstore_client.c b/tools/xenstore/xenstore_client.c
index 9fcd3d2f9e..c5d0b18106 100644
--- a/tools/xenstore/xenstore_client.c
+++ b/tools/xenstore/xenstore_client.c
@@ -140,7 +140,7 @@ static int show_whole_path = 0;
 
 #define MIN(a, b) (((a) < (b))? (a) : (b))
 
-static void do_ls(struct xs_handle *h, char *path, int cur_depth, int 
show_perms)
+static void do_ls(struct xs_handle *h, char *path, int cur_depth, int 
show_perms, int ignore_errors)
 {
     char **e;
     char *newpath, *val;
@@ -151,7 +151,13 @@ static void do_ls(struct xs_handle *h, char *path, int 
cur_depth, int show_perms
     e = xs_directory(h, XBT_NULL, path, &num);
     if (e == NULL) {
         if (cur_depth && (errno == ENOENT || errno == EACCES)) {
-            /* If a node disappears while recursing, silently move on. */
+            /*
+             * If a node disappears or becomes inaccessible while traversing,
+             * only print an error if previous operations on this node haven't
+             * done do. Then move on.
+             */
+            if (!ignore_errors)
+                warn("xs_directory (%s)", path);
             return;
         }
 
@@ -197,7 +203,7 @@ static void do_ls(struct xs_handle *h, char *path, int 
cur_depth, int show_perms
 
         /* Print value */
         if (val == NULL) {
-            printf(":\n");
+            printf(": (%s)", strerror(errno));
         }
         else {
             if (max_width < (linewid + len + TAG_LEN)) {
@@ -222,7 +228,10 @@ static void do_ls(struct xs_handle *h, char *path, int 
cur_depth, int show_perms
         if (show_perms) {
             perms = xs_get_permissions(h, XBT_NULL, newpath, &nperms);
             if (perms == NULL) {
-                warn("\ncould not access permissions for %s", e[i]);
+                /* Don't repeat an error message if xs_read() already failed */
+                if (val)
+                    warn("could not access permissions for %s", e[i]);
+                val = NULL;
             }
             else {
                 int i;
@@ -239,7 +248,7 @@ static void do_ls(struct xs_handle *h, char *path, int 
cur_depth, int show_perms
 
         putchar('\n');
             
-        do_ls(h, newpath, cur_depth+1, show_perms); 
+        do_ls(h, newpath, cur_depth+1, show_perms, !val);
     }
     free(e);
     free(newpath);
@@ -448,7 +457,7 @@ perform(enum mode mode, int optind, int argc, char **argv, 
struct xs_handle *xsh
             break;
         }
         case MODE_ls: {
-            do_ls(xsh, argv[optind], 0, prefix);
+            do_ls(xsh, argv[optind], 0, prefix, 0);
             optind++;
             break;
         }

Attachment: smime.p7s
Description: S/MIME cryptographic signature

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