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

[Juergen Gross <jgross@xxxxxxxx>]

On 04/03/2019 16:10, 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?

ENOENT and EACCES could be the result of races, so continuation is
fine as the result is correct (or has been correct at some time).

The other cases would omit some entries so the result would be
incomplete. So aborting is the correct decision IMO (as long as an
information is issued that the operation was aborted due to an error).


Juergen

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