[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] usb-mtp: fix return status of delete
From: |
Bandan Das |
Subject: |
Re: [Qemu-devel] [PATCH] usb-mtp: fix return status of delete |
Date: |
Mon, 11 Mar 2019 19:02:19 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Peter Maydell <address@hidden> 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
- [Qemu-devel] [PATCH] usb-mtp: fix return status of delete, (continued)
- [Qemu-devel] [PATCH] usb-mtp: fix return status of delete, Bandan Das, 2019/03/08
- Re: [Qemu-devel] [PATCH] usb-mtp: fix return status of delete, Peter Maydell, 2019/03/09
- Re: [Qemu-devel] [PATCH] usb-mtp: fix return status of delete, Bandan Das, 2019/03/11
- Re: [Qemu-devel] [PATCH] usb-mtp: fix return status of delete, Peter Maydell, 2019/03/11
- Re: [Qemu-devel] [PATCH] usb-mtp: fix return status of delete, Bandan Das, 2019/03/11
- Re: [Qemu-devel] [PATCH] usb-mtp: fix return status of delete, Peter Maydell, 2019/03/11
- Re: [Qemu-devel] [PATCH] usb-mtp: fix return status of delete, Bandan Das, 2019/03/11
- Re: [Qemu-devel] [PATCH] usb-mtp: fix return status of delete, Peter Maydell, 2019/03/11
- Re: [Qemu-devel] [PATCH] usb-mtp: fix return status of delete, Bandan Das, 2019/03/11
- Re: [Qemu-devel] [PATCH] usb-mtp: fix return status of delete, Peter Maydell, 2019/03/11
- Re: [Qemu-devel] [PATCH] usb-mtp: fix return status of delete,
Bandan Das <=
- Re: [Qemu-devel] [PULL 3/4] usb-mtp: prevent null dereference while deleting objects, Peter Maydell, 2019/03/09
Re: [Qemu-devel] [PULL 0/4] Usb 20190307 patches, Peter Maydell, 2019/03/07