Peter Maydell <peter.mayd...@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