Re: [Qemu-devel] [PATCH] usb-mtp: fix return status of delete

[Peter Maydell]

On Mon, 11 Mar 2019 at 17:39, Bandan Das <address@hidden> wrote:
>
> Peter Maydell <address@hidden> writes:
>
> > On Mon, 11 Mar 2019 at 16:43, Bandan Das <address@hidden> wrote:
> >> Peter Maydell <address@hidden> writes:
> >> > On Mon, 11 Mar 2019 at 16:14, Bandan Das <address@hidden> wrote:
> >> > Generally, if you have multiple bits X, Y in a return
> >> > value, they should be independent. Sometimes we define
> >> > a convenience value Z that's X | Y, but then Z should
> >> > have a name that indicates that it's really doing both
> >> > X and Y (for instance often a READWRITE constant will
> >> > be READ | WRITE). In this case, I don't see why
> >> > PARTIAL_DELETE would be a sensible name to indicate
> >> > "both ALL_DELETE and also READ_ONLY" -- if we only
> >> > partially did a delete why do we set the ALL_DELETE bit ?
> >> >
> >>
> >> Because during a recursive call, we were able to successfully
> >> delete objects(s) for the previous call but for "this"
> >> set of objects, it failed which is supposed to return a
> >> partial_delete back.
> >>
> >> Does simply "DELETE" instead of "ALL_DELETE" seem less
> >> confusing ? I definitely want to keep PARTIAL_DELETE the
> >> way it is simply because it's easier to refer back
> >> to the spec that way.
> >
> > I think this would be easier to answer if you answered
> > this question:
> >
> >> > It might be useful to take a step back -- what are
> >> > the different possible outcomes from this function that
> >> > we need to distinguish, and when should we be returning
> >> > which outcome?
> >
> They are what the variable names signify.

That doesn't get me any further forward, I'm afraid.

Looking at the code, we seem to have:
 * for any particular node, either we can delete it
   or we can't
 * we also iterate through each child node recursively

So we have to combine together the "deleted this"
and "failed to delete this" information for the whole tree.
In which conditions should we end up with which RES_*
result code? It seems plausible that we want RES_OK
only if every deletion attempt in the tree succeeded.
But what about the others? Is it supposed to be
RES_PARTIAL_DELETE if some deletions succeeded and
some failed, and RES_STORE_READ_ONLY if every single
deletion failed ?

thanks
-- PMM