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

[Bandan Das <bsd@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:
...
>> Ok, this is easier. Now, I know what you are referring to
>> instead of guessing what and how I should be explainng.
>>
>> What you said is essentially correct. When deleting a
>> single object that's a file, the return value would either
>> be OK or STORE_READ_ONLY.
>>
>> When deleting an object which is a folder, Qemu goes through
>> its contents. If it succeeds in deleting all the content objects,
>> the result is success. If one or some objects couldn't be deleted for
>> whatever reason, the result is RES_PARTIAL_DELETE and if none
>> of the objects could be deleted, Qemu returns a READ_ONLY.
>>
>> In usb_mtp_object_delete(), based on the return value of this
>> function, we set s->result appropriately.
>
> OK. So we can do this with a return value where the
> two relevant bits indicate:
>  * bit 0: We had at least one successful deletion
>  * bit 1: We had at least one failed deletion
>
> and then the correct RES value is:
>  * only bit 0 set: READ_ONLY
>  * only bit 1 set: OK
>  * both bits set: PARTIAL_DELETE
>  * neither bit set: can't happen
>
> This is what your patch does, I think, but you've named
> the "at least one deletion succeeded" bit "ALL_DELETE"
> (even though it can be set in a return value where not
> all of the deletions succeeded) and the "at least one
> deletion failed" bit "READ_ONLY" (even though it can
> be set in a return value where some deletions succeeded),
> which is what I found confusing.
>
> I think the code would be easier to understand if we:
>  * picked clearer names for the bits, like
>    DELETE_SUCCESS and DELETE_FAILURE
>  * had a comment explaining what the return value
>    of the function means, something like:
>
> /*
>  * Delete the object @o and all its children. In the
>  * return value, the DELETE_SUCCESS bit is set if at
>  * least one of the deletions succeeded, and the
>  * DELETE_FAILURE bit is set if at least one of the
>  * deletions failed. If the tree of objects was only
>  * partially deleted then both bits will be set.
>  */
>
> But really the second of these is the more important:
> slightly confusing naming is OK if there is a good
> comment explaining what is going on (and my suggested
> bit flag names don't really stand on their own without
> any explanation either). So if you strongly prefer your names
> for the bits that's ok, but please do add a comment,
> either at the top of the function or documenting
> the meaning of the enum values.
>
Peter, thank you for the thorough review, I really appreciate it.
I definitely want to make this code less confusing to the next
potential reviewer. I will address all your suggestions in the
next version of the patch.

Bandan

> thanks
> -- PMM