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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Mar 04, 2019 at 03:10:34PM +0000, David Woodhouse wrote:
> On Mon, 2019-03-04 at 15:51 +0100, Juergen Gross wrote:
> > On 04/03/2019 15:31, David Woodhouse wrote:
> > > On Mon, 2019-03-04 at 14:18 +0000, Wei Liu wrote:
> > > > CC Ian as well.
> > > > 
> > > > It would be better if you run ./scripts/get_maintainers.pl on
> > > > your
> > > > patches in the future to CC the correct people.
> > > 
> > > Will do; thanks.
> > > 
> > > > On Fri, Mar 01, 2019 at 12:16:56PM +0000, David Woodhouse wrote:
> > > > > From: David Woodhouse <dwmw@xxxxxxxxxxxx>
> > > > > 
> > > > > When recursing, a node sometimes disappears. Deal with it and
> > > > > move on
> > > > > instead of aborting and failing to print the rest of what was
> > > > > requested.
> > > > >     
> > > > > Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
> > > > > ---
> > > > > And thus did an extremely sporadic "not going to delete that
> > > > > device
> > > > > because it already doesn't exist" failure mode become painfully
> > > > > obvious
> > > > > in retrospect...
> > > > > 
> > > > > diff --git a/tools/xenstore/xenstore_client.c
> > > > > b/tools/xenstore/xenstore_client.c
> > > > > index 3afc630ab8..c089d60a2a 100644
> > > > > --- a/tools/xenstore/xenstore_client.c
> > > > > +++ b/tools/xenstore/xenstore_client.c
> > > > > @@ -153,8 +153,13 @@ static void do_ls(struct xs_handle *h,
> > > > > char
> > > > > *path,
> > > > > int cur_depth, int show_perms
> > > > >        err(1, "malloc in do_ls");
> > > > >  
> > > > >      e = xs_directory(h, XBT_NULL, path, &num);
> > > > > -    if (e == NULL)
> > > > > -        err(1, "xs_directory (%s)", path);
> > > > > +    if (e == NULL) {
> > > > > +        if (!cur_depth)
> > > > > +            err(1, "xs_directory (%s)", path);
> > > > > +
> > > > > +        /* If a node disappears while recursing, silently move
> > > > > on.
> > > > > */
> > > > > +        num = 0;
> > > > > +    }
> > > > 
> > > > Can you check if the errno is ENOENT? I would rather not ignore
> > > > other
> > > > types of errors if possible.
> > > 
> > > Under what circumstances is it correct for xenstore-ls to abort
> > > immediately and not attempt to print anything more?
> > > 
> > > I'm sure there are circumstances where the world is so hosed that
> > > it'll
> > > keep getting errors and *fail* to print anything more. But to
> > > deliberately bail out? Should that really be restricted to ENOENT?
> > 
> > EACCES seems to be another candidate for trying to continue.
> > 
> > EINVAL, ENOMEM and EIO should never occur, so aborting the operation
> > would be okay IMO.
> 
> If you get one of those errors for a given path, but then everything
> else afterwards works fine.... do you really *want* to abort and not
> print the later ones? I agree it would be OK — at least tolerable — to
> abort. But is it really the most desired outcome?
> 

To me it is just a bit weird to guard with cur_depth -- if you really
want to continue at all cost, why don't you make it really continue at
all cost?

Also you mentioned "a node disappears", I thought that was the case you
cared about, hence my suggestion.

Ultimately I'm too fussed either way: xenstore-ls is a dom0 utility
which helps debugging. People who use / need it most get to decide what
its behaviour should be.

Wei.

> 



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