[Qemu-devel] [PATCH v4 0/3] misc usb-mtp fixes

[Qemu-devel] [PATCH v4 0/3] misc usb-mtp fixes

[Bandan Das]

v4:
  Added 1/3: <jpgva0nuddm.fsf@linux.bootlegged.copy>  
v3:
  2/2: Fix indentation
       Add back sending RES_OK for success
v2:
  1/2: Add Reviewed-by tag
  2/2: remove extra vars and directly call usb_mtp_queue_result
  
The first patch removes a unnecessary function
and the second is just a code reorg of usb_mtp_write_data
to make it less confusing. Applies on top of
[PATCH v3] usb-mtp: fix return status of delete
Message-ID: 

Bandan Das (3):
  usb-mtp: fix return status of delete
  usb-mtp: remove usb_mtp_object_free_one
  usb-mtp: refactor the flow of usb_mtp_write_data

 hw/usb/dev-mtp.c | 133 +++++++++++++++++++++++------------------------
 1 file changed, 66 insertions(+), 67 deletions(-)

-- 
2.19.2

[Qemu-devel] [PATCH v4 1/3] usb-mtp: fix return status of delete

[Bandan Das]

Spotted by Coverity: CID 1399414

mtp delete allows the return status of delete succeeded,
partial_delete or readonly - when none of the objects could be
deleted. Give more meaningful names to return values of the
delete function.

Some initiators recurse over the objects themselves. In that case,
only READ_ONLY can be returned.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 62 ++++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 28 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 06e376bcd2..91b820baaf 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1135,11 +1135,19 @@ static MTPData *usb_mtp_get_object_prop_value(MTPState *s, MTPControl *c,
     return d;
 }
 
-/* Return correct return code for a delete event */
+/*
+ * Return values when object @o is deleted.
+ * If at least one of the deletions succeeded,
+ * DELETE_SUCCESS is set and if at least one
+ * of the deletions failed, DELETE_FAILURE is
+ * set. Both bits being set (DELETE_PARTIAL)
+ * signifies a  RES_PARTIAL_DELETE being sent
+ * back to the initiator.
+ */
 enum {
-    ALL_DELETE,
-    PARTIAL_DELETE,
-    READ_ONLY,
+    DELETE_SUCCESS = (1 << 0),
+    DELETE_FAILURE = (1 << 1),
+    DELETE_PARTIAL = (DELETE_FAILURE | DELETE_SUCCESS),
 };
 
 /* Assumes that children, if any, have been already freed */
@@ -1155,8 +1163,7 @@ static void usb_mtp_object_free_one(MTPState *s, MTPObject *o)
 static int usb_mtp_deletefn(MTPState *s, MTPObject *o, uint32_t trans)
 {
     MTPObject *iter, *iter2;
-    bool partial_delete = false;
-    bool success = false;
+    int ret = 0;
 
     /*
      * TODO: Add support for Protection Status
@@ -1165,34 +1172,28 @@ static int usb_mtp_deletefn(MTPState *s, MTPObject *o, uint32_t trans)
     QLIST_FOREACH(iter, &o->children, list) {
         if (iter->format == FMT_ASSOCIATION) {
             QLIST_FOREACH(iter2, &iter->children, list) {
-                usb_mtp_deletefn(s, iter2, trans);
+                ret |= usb_mtp_deletefn(s, iter2, trans);
             }
         }
     }
 
     if (o->format == FMT_UNDEFINED_OBJECT) {
         if (remove(o->path)) {
-            partial_delete = true;
+            ret |= DELETE_FAILURE;
         } else {
             usb_mtp_object_free_one(s, o);
-            success = true;
+            ret |= DELETE_SUCCESS;
         }
     } else if (o->format == FMT_ASSOCIATION) {
         if (rmdir(o->path)) {
-            partial_delete = true;
+            ret |= DELETE_FAILURE;
         } else {
             usb_mtp_object_free_one(s, o);
-            success = true;
+            ret |= DELETE_SUCCESS;
         }
     }
 
-    if (success && partial_delete) {
-        return PARTIAL_DELETE;
-    }
-    if (!success && partial_delete) {
-        return READ_ONLY;
-    }
-    return ALL_DELETE;
+    return ret;
 }
 
 static void usb_mtp_object_delete(MTPState *s, uint32_t handle,
@@ -1226,19 +1227,24 @@ static void usb_mtp_object_delete(MTPState *s, uint32_t handle,
     }
 
     ret = usb_mtp_deletefn(s, o, trans);
-    if (ret == PARTIAL_DELETE) {
-        usb_mtp_queue_result(s, RES_PARTIAL_DELETE,
-                             trans, 0, 0, 0, 0);
-        return;
-    } else if (ret == READ_ONLY) {
-        usb_mtp_queue_result(s, RES_STORE_READ_ONLY, trans,
-                             0, 0, 0, 0);
-        return;
-    } else {
+    switch (ret) {
+    case DELETE_SUCCESS:
         usb_mtp_queue_result(s, RES_OK, trans,
                              0, 0, 0, 0);
-        return;
+        break;
+    case DELETE_FAILURE:
+        usb_mtp_queue_result(s, RES_PARTIAL_DELETE,
+                             trans, 0, 0, 0, 0);
+        break;
+    case DELETE_PARTIAL:
+        usb_mtp_queue_result(s, RES_PARTIAL_DELETE,
+                             trans, 0, 0, 0, 0);
+        break;
+    default:
+        g_assert_not_reached();
     }
+
+    return;
 }
 
 static void usb_mtp_command(MTPState *s, MTPControl *c)
-- 
2.19.2

[Qemu-devel] [PATCH v4 2/3] usb-mtp: remove usb_mtp_object_free_one

[Bandan Das]

This function is used in the delete path only and can
be replaced by a call to usb_mtp_object_free.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 91b820baaf..4dc1317e2e 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1150,16 +1150,6 @@ enum {
     DELETE_PARTIAL = (DELETE_FAILURE | DELETE_SUCCESS),
 };
 
-/* Assumes that children, if any, have been already freed */
-static void usb_mtp_object_free_one(MTPState *s, MTPObject *o)
-{
-    assert(o->nchildren == 0);
-    QTAILQ_REMOVE(&s->objects, o, next);
-    g_free(o->name);
-    g_free(o->path);
-    g_free(o);
-}
-
 static int usb_mtp_deletefn(MTPState *s, MTPObject *o, uint32_t trans)
 {
     MTPObject *iter, *iter2;
@@ -1181,14 +1171,14 @@ static int usb_mtp_deletefn(MTPState *s, MTPObject *o, uint32_t trans)
         if (remove(o->path)) {
             ret |= DELETE_FAILURE;
         } else {
-            usb_mtp_object_free_one(s, o);
+            usb_mtp_object_free(s, o);
             ret |= DELETE_SUCCESS;
         }
     } else if (o->format == FMT_ASSOCIATION) {
         if (rmdir(o->path)) {
             ret |= DELETE_FAILURE;
         } else {
-            usb_mtp_object_free_one(s, o);
+            usb_mtp_object_free(s, o);
             ret |= DELETE_SUCCESS;
         }
     }
-- 
2.19.2

[Qemu-devel] [PATCH v4 3/3] usb-mtp: refactor the flow of usb_mtp_write_data

[Bandan Das]

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

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 57 +++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 4dc1317e2e..0afb926719 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 =
@@ -1616,26 +1616,33 @@ 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) {
+                    usb_mtp_queue_result(s, RES_OK, d->trans, 3,
+                                         QEMU_STORAGE_ID,
+                                         s->dataset.parent_handle,
+                                         handle);
+                    goto close;
+                }
+                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 +1654,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 +1671,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,
+                             0, 0, 0, 0);
+    }
+close:
     /*
      * The write dataset is kept around and freed only
      * on success or if another write request comes in
@@ -1683,12 +1693,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 +1733,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 +1817,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:
-- 
2.19.2

Re: [Qemu-devel] [PATCH v4 2/3] usb-mtp: remove usb_mtp_object_free_one

[Gerd Hoffmann]

On Mon, Apr 01, 2019 at 05:17:11PM -0400, Bandan Das wrote:
> This function is used in the delete path only and can
> be replaced by a call to usb_mtp_object_free.

Queued patch 1+2, leaving 3 for later.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v4 0/3] misc usb-mtp fixes

[Peter Maydell]

On Mon, 1 Apr 2019 at 22:17, Bandan Das <bsd@redhat.com> wrote:
>
> v4:
>   Added 1/3: <jpgva0nuddm.fsf@linux.bootlegged.copy>
> v3:
>   2/2: Fix indentation
>        Add back sending RES_OK for success
> v2:
>   1/2: Add Reviewed-by tag
>   2/2: remove extra vars and directly call usb_mtp_queue_result
>
> The first patch removes a unnecessary function
> and the second is just a code reorg of usb_mtp_write_data
> to make it less confusing. Applies on top of
> [PATCH v3] usb-mtp: fix return status of delete
> Message-ID:
>
> Bandan Das (3):
>   usb-mtp: fix return status of delete
>   usb-mtp: remove usb_mtp_object_free_one
>   usb-mtp: refactor the flow of usb_mtp_write_data

Hi Bandan, Gerd -- what's the status of this patchset?
I think this is the one that fixes the CID1399415
Coverity issue about usb_mtp_write_data() return values, right?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 0/3] misc usb-mtp fixes

[Gerd Hoffmann]

> > Bandan Das (3):
> >   usb-mtp: fix return status of delete
> >   usb-mtp: remove usb_mtp_object_free_one
> >   usb-mtp: refactor the flow of usb_mtp_write_data
> 
> Hi Bandan, Gerd -- what's the status of this patchset?
> I think this is the one that fixes the CID1399415
> Coverity issue about usb_mtp_write_data() return values, right?

1+2 fixes where merged during 4.0 freeze, 3 left for post-freeze.
It can follow now, queued up, next usb pull will have it.

thanks for the reminder,
  Gerd