[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-devel] [PATCH 2/2] tools/xenstore: Accumulate errors in xenstore-ls and exit appropriately [and 1 more messages]



David Woodhouse writes ("Re: [Xen-devel] [PATCH 2/2] tools/xenstore: Accumulate 
errors in xenstore-ls and exit appropriately"):
> It's patch 1 which I really care about; this part is just yak shaving
> at Ian's prompting.

Hi.  Thanks for working on this cleanup.

I confess that while reviewing your code I felt a bit confused and
thickheaded.  (I have had a difficult week.)  So I think what I am
about to say may not be as useful or true as it ought to be....

You'll see I've put my R-b on patch 1.  Despite that, given what
appears here in patch 2, I think you might want to try to look into
using xs_transaction_{start,end} in xenstore-ls.  That might make
things simpler.

In particular I think this because I'm not sure I fully understand the
implications of when to ignore EACCES errors, purportedly on the
grounds that they might be caused by concurrent updates to xenstore.
My inclination is to think that this can't be right.  But doing
everything in a transaction would completely eliminate this issue.


Regardless of that, it is still a bug that xenstore-ls ignores errors
far too much and I very much welcome your efforts to fix this in 2/.

I don't know if I already explained my overall theory about this but:
ISTM that xenstore-ls should exit 0 iff it was able to find and print
all the information requested.  If it exits non-0 it should have
printed at least one thing to stderr.


David Woodhouse writes ("[PATCH 2/2] tools/xenstore: Accumulate errors in 
xenstore-ls and exit appropriately"):
> From: David Woodhouse <dwmw@xxxxxxxxxxxx>
> 
> Report only one error to stderr for each node, regardless of whether it's
> xs_read, xs_get_permissions or xs_directory on the child that fails.
> 
> Always exit with a non-zero code if any failure happens, reporting the
> last error that occurred.

I think though, that this patch has some remnants of previous
iterations in it.

> -static void do_ls(struct xs_handle *h, char *path, int cur_depth, int 
> show_perms)
> +static int do_ls(struct xs_handle *h, char *path, int cur_depth, int 
> show_perms, int error, int ignore_errors)
>  {
>      char **e;
>      char *newpath, *val;
> @@ -150,9 +150,16 @@ 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) {
> -            /* If a node disappears while recursing, silently move on. */
> -            return;

ISTM that this code ought to be retained.  It is still the case that
you want to ignore ENOENT.  (Unless you do the transaction thing.)

> +        if (cur_depth && (errno == ENOENT || errno == EACCES)) {
> +            /*
> +             * 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.
> +             */
> +            error = errno;
> +            if (!ignore_errors)
> +                warn("xs_directory (%s)", path);
> +            return error;

So this branch is just for EACCES ?  What is the justification for
handling EACCES specially ?  Maybe it is the only "expected" error ?

> @@ -197,7 +204,8 @@ static void do_ls(struct xs_handle *h, char *path, int 
> cur_depth, int show_perms
>  
>          /* Print value */
>          if (val == NULL) {
> -            printf(":\n");
> +            error = errno;
> +            printf(": (%s)", strerror(error));

Well done for making the output unambiguous.  `= "..."' vs `: (...)'

> @@ -222,7 +230,11 @@ 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]);
> +                error = errno;
> +                val = NULL;
> +                /* Don't repeat an error message if xs_read() already failed 
> */
> +                if (val)
> +                    warn("could not access permissions for %s", e[i]);

Maybe this wants the same ENOENT handling as before ?  (Unless you do
xs_transaction_*.)

IDK what the rules are for xs_get_permissions.[1] Does it require read
access ?  If so then I think it might be better to simply skip the
xs_get_permissions call if val==NULL.  Especially if you do the
xs_transaction_* thing - since in that case the xs_get_permissions
call seems doomed.

I hope this review was of some use.  Please chat to me on irc or reply
by email if it doesn't seem to make sense.

Ian.

[1] The in-tree docs refer here:
  https://wiki.xen.org/wiki/XenBus#Permissions
but that part of the wiki page does not say what permissions are
needed for GET_PERMS and SET_PERMS themselves.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.