[2/2] usb-mtp: refactor the flow of usb_mtp_write_data

Commit Message

Bandan Das March 19, 2019, 9:46 p.m. UTC

There's no functional change but the flow is (hopefully)
more consistent for both file and folder object types.

Signed-off-by: Bandan Das <[e-mail]>
---
 hw/usb/dev-mtp.c | 58 +++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 28 deletions(-)

Comments

Peter Maydell March 26, 2019, 10:58 a.m. UTC | #1

On Tue, 19 Mar 2019 at 21:46, Bandan Das <[e-mail]> wrote:
>
> There's no functional change but the flow is (hopefully)
> more consistent for both file and folder object types.
>
> Signed-off-by: Bandan Das <[e-mail]>
> ---
>  hw/usb/dev-mtp.c | 58 +++++++++++++++++++++++++-----------------------
>  1 file changed, 30 insertions(+), 28 deletions(-)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 4dc1317e2e..ced3a22384 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -1599,7 +1599,7 @@ static int usb_mtp_update_object(MTPObject *parent, char *name)
>      return ret;
>  }
>
> -static int usb_mtp_write_data(MTPState *s)
> +static void usb_mtp_write_data(MTPState *s, uint32_t handle)
>  {
>      MTPData *d = s->data_out;
>      MTPObject *parent =
> @@ -1607,7 +1607,7 @@ static int usb_mtp_write_data(MTPState *s)
>      char *path = NULL;
>      uint64_t rc;
>      mode_t mask = 0644;
> -    int ret = 0;
> +    int ret = 0, arg4 = 0, arg5 = 0, arg6 = 0, arg7 = 0;
>
>      assert(d != NULL);
>
> @@ -1616,26 +1616,32 @@ static int usb_mtp_write_data(MTPState *s)
>          if (!parent || !s->write_pending) {
>              usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
>                  0, 0, 0, 0);
> -        return 1;
> +        return;
>          }
>
>          if (s->dataset.filename) {
>              path = g_strdup_printf("%s/%s", parent->path, s->dataset.filename);
>              if (s->dataset.format == FMT_ASSOCIATION) {
>                  ret = mkdir(path, mask);
> -                goto free;
> +                if (!ret) {
> +                    arg4 = 3;
> +                    arg5 = QEMU_STORAGE_ID;
> +                    arg6 = s->dataset.parent_handle;
> +                    arg7 = handle;
> +                }
> +                goto done;

We only use the arg4/5/6/7 as anything other than 0 here,
so I think it would be clearer to write
   if (!ret) {
       usb_mtp_queue_result(s, RES_OK, d->trans, 3,
                            QEMU_STORAGE_ID, s->dataset.parent_handle,
                            handle);
       goto close;
  }

and then your code at the 'done' label can just unconditionally
use 0,0,0,0.

thanks
-- PMM

Patch

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 4dc1317e2e..ced3a22384 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1599,7 +1599,7 @@  static int usb_mtp_update_object(MTPObject *parent, char *name)
     return ret;
 }
 
-static int usb_mtp_write_data(MTPState *s)
+static void usb_mtp_write_data(MTPState *s, uint32_t handle)
 {
     MTPData *d = s->data_out;
     MTPObject *parent =
@@ -1607,7 +1607,7 @@  static int usb_mtp_write_data(MTPState *s)
     char *path = NULL;
     uint64_t rc;
     mode_t mask = 0644;
-    int ret = 0;
+    int ret = 0, arg4 = 0, arg5 = 0, arg6 = 0, arg7 = 0;
 
     assert(d != NULL);
 
@@ -1616,26 +1616,32 @@  static int usb_mtp_write_data(MTPState *s)
         if (!parent || !s->write_pending) {
             usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
                 0, 0, 0, 0);
-        return 1;
+        return;
         }
 
         if (s->dataset.filename) {
             path = g_strdup_printf("%s/%s", parent->path, s->dataset.filename);
             if (s->dataset.format == FMT_ASSOCIATION) {
                 ret = mkdir(path, mask);
-                goto free;
+                if (!ret) {
+                    arg4 = 3;
+                    arg5 = QEMU_STORAGE_ID;
+                    arg6 = s->dataset.parent_handle;
+                    arg7 = handle;
+                }
+                goto done;
             }
+
             d->fd = open(path, O_CREAT | O_WRONLY |
                          O_CLOEXEC | O_NOFOLLOW, mask);
             if (d->fd == -1) {
-                usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
-                                     0, 0, 0, 0);
+                ret = 1;
                 goto done;
             }
 
             /* Return success if initiator sent 0 sized data */
             if (!s->dataset.size) {
-                goto success;
+                goto done;
             }
             if (d->length != MTP_WRITE_BUF_SZ && !d->pending) {
                 d->write_status = WRITE_END;
@@ -1647,13 +1653,12 @@  static int usb_mtp_write_data(MTPState *s)
         rc = write_retry(d->fd, d->data, d->data_offset,
                          d->offset - d->data_offset);
         if (rc != d->data_offset) {
-            usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
-                                 0, 0, 0, 0);
+            ret = 1;
             goto done;
         }
         if (d->write_status != WRITE_END) {
             g_free(path);
-            return ret;
+            return;
         } else {
             /*
              * Return an incomplete transfer if file size doesn't match
@@ -1665,16 +1670,20 @@  static int usb_mtp_write_data(MTPState *s)
                 usb_mtp_update_object(parent, s->dataset.filename)) {
                 usb_mtp_queue_result(s, RES_INCOMPLETE_TRANSFER, d->trans,
                                      0, 0, 0, 0);
-                goto done;
+                goto close;
             }
         }
     }
 
-success:
-    usb_mtp_queue_result(s, RES_OK, d->trans,
-                         0, 0, 0, 0);
-
 done:
+    if (ret) {
+        usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
+                             0, 0, 0, 0);
+    } else {
+        usb_mtp_queue_result(s, RES_OK, d->trans,
+                             arg4, arg5, arg6, arg7);
+    }
+close:
     /*
      * The write dataset is kept around and freed only
      * on success or if another write request comes in
@@ -1683,12 +1692,10 @@  done:
         close(d->fd);
         d->fd = -1;
     }
-free:
     g_free(s->dataset.filename);
     s->dataset.size = 0;
     g_free(path);
     s->write_pending = false;
-    return ret;
 }
 
 static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen)
@@ -1725,16 +1732,11 @@  static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen)
     s->write_pending = true;
 
     if (s->dataset.format == FMT_ASSOCIATION) {
-        if (usb_mtp_write_data(s)) {
-            /* next_handle will be allocated to the newly created dir */
-            usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
-                                 0, 0, 0, 0);
-            return;
-        }
+        usb_mtp_write_data(s, next_handle);
+    } 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,
@@ -1814,14 +1816,14 @@  static void usb_mtp_get_data(MTPState *s, mtp_container *container,
             } else {
                 d->write_status = WRITE_START;
             }
-            usb_mtp_write_data(s);
+            usb_mtp_write_data(s, 0);
             usb_mtp_data_free(s->data_out);
             s->data_out = NULL;
             return;
         }
         if (d->data_offset == d->length) {
             d->pending = true;
-            usb_mtp_write_data(s);
+            usb_mtp_write_data(s, 0);
         }
         break;
     default: