Re: [Qemu-devel] [PULL 2/4] usb-mtp: fix some usb_mtp_write_data return paths

[Bandan Das]

Peter Maydell <address@hidden> writes:

> On Fri, 8 Mar 2019 at 19:39, Bandan Das <address@hidden> wrote:
>>
>> Peter Maydell <address@hidden> writes:
>> > But the two places in usb_mtp_get_data() that call
>> > usb_mtp_write_metadata() still don't check its return
>> > value: don't they need to handle failure too?
>> >
>> I believe this is ok because:
>> The return value of usb_mtp_write_data is only used to check if mkdir
>> failed and update s->result in usb_mtp_write_metadata().
>> The next time usb_mtp_handle_data is called, it will process s->result.
>
> I think I still don't really understand the error handling
> in this function. Why do we deal with mkdir() failing by
> having the function return -1 and then doing
>  usb_mtp_queue_result(s, RES_STORE_FULL, ...)
> (but only at one callsite), whereas for open() or write()
> failing we do the usb_mtp_queue_result(s, RES_STORE_FULL, ...)
> inside the function itself?
>

usb_mtp_write_metadata() handles the sendobjectinfo phase where the
initiator sends the metadata associated with the incoming object.
For a file, the name and the size is sent and once the responder sends
back OK, the initiator starts the sendobject phase. For a folder,
the name of the folder is sent with size being 0, and
no sendobject phase follows.

So, the reason I am sending back the return
value is because for a folder, I want to send a success or a failure
based on whether mkdir succeeded but for a file object, I want to return
success so that the next phase can continue. Is this rewrite better ?

 static void usb_mtp_object_delete(MTPState *s, uint32_t handle,
@@ -1674,7 +1666,13 @@ static void usb_mtp_write_data(MTPState *s)
         if (s->dataset.filename) {
             path = g_strdup_printf("%s/%s", parent->path, s->dataset.filename);
             if (s->dataset.format == FMT_ASSOCIATION) {
-                d->fd = mkdir(path, mask);
+                if (mkdir(path, mask)) {
+                    usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
+                                         0, 0, 0, 0);
+                } else {
+                    usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
+                                         0, 0, 0, 0);
+                }
                 goto free;
             }
             d->fd = open(path, O_CREAT | O_WRONLY | O_CLOEXEC | O_NOFOLLOW, 
mask);
@@ -1769,17 +1767,10 @@ static void usb_mtp_write_metadata(MTPState *s, 
uint64_t dlen)
 
     if (s->dataset.format == FMT_ASSOCIATION) {
         usb_mtp_write_data(s);
-        /* next_handle will be allocated to the newly created dir */
-        if (d->fd == -1) {
-            usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
-                                 0, 0, 0, 0);
-            return;
-        }
-        d->fd = -1;
+    } else {
+        usb_mtp_queue_result(s, RES_OK, d->trans, 3, QEMU_STORAGE_ID,
+                             s->dataset.parent_handle, next_handle);
     }
-
-    usb_mtp_queue_result(s, RES_OK, d->trans, 3, QEMU_STORAGE_ID,
-                         s->dataset.parent_handle, next_handle);
 }
 
 static void usb_mtp_get_data(MTPState *s, mtp_container *container,



> thanks
> -- PMM