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

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

David Woodhouse writes ("Re: [PATCH v2] tools/xenstore: Do not abort 
xenstore-ls if a node disappears while iterating"):
> On Wed, 2019-08-07 at 16:37 +0100, Ian Jackson wrote:
> > 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.

Sorry, perhaps I wasn't clear enough.  I am particularly concerned
about the relationship between possible partial output, and the exit
status.

> (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

I'm pretty sure that here xenstore-ls exited nonzero.

> 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.

That's freaky but at the very least we shouldn't make it worse.

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

Presumably it fails to print permissions information, too.  So some
tool which is (1) checking the exit status and (2) using the output
will not be misled.

I think in both of these cases it exits zero (assuming nothing else
goes wrong).

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

In this situation it exits nonzero.  Partial output with a nonzero
exit status is not ideal, but it is not incorrect.  Omitted output,
with a zero exit status, is a potential data loss bug and must be
avoided.

(And I think this is true even if there is a message to stderr.)

> 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.

Can you explain to me what your needs are for all this ?  I want
xenstore-ls to be able to do what you want, but I don't want to impose
a lot of work on you.  I also want the correctness property I mention
above.

My view of the ideal situation would be for xenstore-ls to have
defined exit status values, like

  0    everything went well
  1    not used, reserved because some runtimes etc. invent 1
  2    permissions of at least one node could not be printed
  3    value of at least one node could not be printed
  4    children of at least one node could not be listed,
        nodes may be missing
 127   catastrophe, all bets are off

And maybe some command line options to control which of these
conditions print a message to stderr, and/or to turn some of them into
0 for the caller's convenience.

But this would involve xenstore-ls having a global variable where it
accumulates the error status, and going through the whole program and
fixing the error handling (which currently seems not very good to me).

If you want to do that, then great, let us talk about the details.

If that's too much work then maybe you can do something now which at
least goes in the right direction.

Does that make sense ?

Thanks,
Ian.

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