diff mbox

[1/3] usb-mtp: use a list for keeping track of children

Message ID 1446595225-23608-2-git-send-email-bsd@redhat.com
State New
Headers show

Commit Message

Bandan Das Nov. 4, 2015, midnight UTC 
To support adding/removal of objects, we will need to update
the object cache hierarchy we have built internally. Convert
to using a Qlist for easier management.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 67 +++++++++++++++++++++++++++++++++++++-------------------
 trace-events     |  1 +
 2 files changed, 46 insertions(+), 22 deletions(-)

Comments

Gerd Hoffmann Nov. 5, 2015, 8:24 a.m. UTC | #1 
>  static void usb_mtp_object_free(MTPState *s, MTPObject *o)
>  {
> -    int i;
> +    MTPObject *iter;
> +
> +    if (o) {
> +        trace_usb_mtp_object_free(s->dev.addr, o->handle, o->path);

That also makes usb_mtp_object_free callable with o == NULL.  Makes
sense, but also makes this patch hard to review.  Please consider either
splitting this out into a separate patch or code this as "if (!o)
{ return; }" so the intention of the remaining code doesn't change.

> +static MTPObject *usb_mtp_add_child(MTPState *s, MTPObject *o,
> +                              char *name)
> +{
> +    MTPObject *child =
> +        usb_mtp_object_alloc(s, s->next_handle++, o, name);
> +
> +    if (child) {
> +        trace_usb_mtp_add_child(s->dev.addr, child->handle, child->path);
> +        QLIST_INSERT_HEAD(&o->children, child, list);
> +        o->nchildren++;
> +
> +        if (child->format == FMT_ASSOCIATION) {
> +            QLIST_INIT(&child->children);
> +        }
> +
> +    }
> +
> +    return child;
> +}

Separate patch please.

cheers,
  Gerd
Bandan Das Nov. 5, 2015, 9:23 p.m. UTC | #2 
Gerd Hoffmann <kraxel@redhat.com> writes:

>>  static void usb_mtp_object_free(MTPState *s, MTPObject *o)
>>  {
>> -    int i;
>> +    MTPObject *iter;
>> +
>> +    if (o) {
>> +        trace_usb_mtp_object_free(s->dev.addr, o->handle, o->path);
>
> That also makes usb_mtp_object_free callable with o == NULL.  Makes
> sense, but also makes this patch hard to review.  Please consider either
> splitting this out into a separate patch or code this as "if (!o)
> { return; }" so the intention of the remaining code doesn't change.

Sure, I will split up the call to usb_mtp_object_free() during reset
to a separate patch and change the check here to if (!o).

Bandan

>> +static MTPObject *usb_mtp_add_child(MTPState *s, MTPObject *o,
>> +                              char *name)
>> +{
>> +    MTPObject *child =
>> +        usb_mtp_object_alloc(s, s->next_handle++, o, name);
>> +
>> +    if (child) {
>> +        trace_usb_mtp_add_child(s->dev.addr, child->handle, child->path);
>> +        QLIST_INSERT_HEAD(&o->children, child, list);
>> +        o->nchildren++;
>> +
>> +        if (child->format == FMT_ASSOCIATION) {
>> +            QLIST_INIT(&child->children);
>> +        }
>> +
>> +    }
>> +
>> +    return child;
>> +}
>
> Separate patch please.
>
> cheers,
>   Gerd
diff mbox

Patch

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 809b1cb..37dfa13 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -109,8 +109,9 @@  struct MTPObject {
     char         *path;
     struct stat  stat;
     MTPObject    *parent;
-    MTPObject    **children;
     uint32_t     nchildren;
+    QLIST_HEAD(, MTPObject) children;
+    QLIST_ENTRY(MTPObject) list;
     bool         have_children;
     QTAILQ_ENTRY(MTPObject) next;
 };
@@ -317,18 +318,24 @@  ignore:
 
 static void usb_mtp_object_free(MTPState *s, MTPObject *o)
 {
-    int i;
+    MTPObject *iter;
+
+    if (o) {
+        trace_usb_mtp_object_free(s->dev.addr, o->handle, o->path);
 
-    trace_usb_mtp_object_free(s->dev.addr, o->handle, o->path);
+        QTAILQ_REMOVE(&s->objects, o, next);
+        if (o->parent) {
+            QLIST_REMOVE(o, list);
+            o->parent->nchildren--;
+        }
 
-    QTAILQ_REMOVE(&s->objects, o, next);
-    for (i = 0; i < o->nchildren; i++) {
-        usb_mtp_object_free(s, o->children[i]);
+        QLIST_FOREACH(iter, &o->children, list) {
+            usb_mtp_object_free(s, iter);
+        }
+        g_free(o->name);
+        g_free(o->path);
+        g_free(o);
     }
-    g_free(o->children);
-    g_free(o->name);
-    g_free(o->path);
-    g_free(o);
 }
 
 static MTPObject *usb_mtp_object_lookup(MTPState *s, uint32_t handle)
@@ -343,6 +350,26 @@  static MTPObject *usb_mtp_object_lookup(MTPState *s, uint32_t handle)
     return NULL;
 }
 
+static MTPObject *usb_mtp_add_child(MTPState *s, MTPObject *o,
+                              char *name)
+{
+    MTPObject *child =
+        usb_mtp_object_alloc(s, s->next_handle++, o, name);
+
+    if (child) {
+        trace_usb_mtp_add_child(s->dev.addr, child->handle, child->path);
+        QLIST_INSERT_HEAD(&o->children, child, list);
+        o->nchildren++;
+
+        if (child->format == FMT_ASSOCIATION) {
+            QLIST_INIT(&child->children);
+        }
+
+    }
+
+    return child;
+}
+
 static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
 {
     struct dirent *entry;
@@ -358,16 +385,9 @@  static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
         return;
     }
     while ((entry = readdir(dir)) != NULL) {
-        if ((o->nchildren % 32) == 0) {
-            o->children = g_realloc(o->children,
-                                    (o->nchildren + 32) * sizeof(MTPObject *));
-        }
-        o->children[o->nchildren] =
-            usb_mtp_object_alloc(s, s->next_handle++, o, entry->d_name);
-        if (o->children[o->nchildren] != NULL) {
-            o->nchildren++;
-        }
+        usb_mtp_add_child(s, o, entry->d_name);
     }
+
     closedir(dir);
 }
 
@@ -618,13 +638,15 @@  static MTPData *usb_mtp_get_object_handles(MTPState *s, MTPControl *c,
                                            MTPObject *o)
 {
     MTPData *d = usb_mtp_data_alloc(c);
-    uint32_t i, handles[o->nchildren];
+    uint32_t i = 0, handles[o->nchildren];
+    MTPObject *iter;
 
     trace_usb_mtp_op_get_object_handles(s->dev.addr, o->handle, o->path);
 
-    for (i = 0; i < o->nchildren; i++) {
-        handles[i] = o->children[i]->handle;
+    QLIST_FOREACH(iter, &o->children, list) {
+        handles[i++] = iter->handle;
     }
+    assert(i == o->nchildren);
     usb_mtp_add_u32_array(d, o->nchildren, handles);
 
     return d;
@@ -885,6 +907,7 @@  static void usb_mtp_handle_reset(USBDevice *dev)
 
     trace_usb_mtp_reset(s->dev.addr);
 
+    usb_mtp_object_free(s, QTAILQ_FIRST(&s->objects));
     s->session = 0;
     usb_mtp_data_free(s->data_in);
     s->data_in = NULL;
diff --git a/trace-events b/trace-events
index 72136b9..ba4473d 100644
--- a/trace-events
+++ b/trace-events
@@ -552,6 +552,7 @@  usb_mtp_op_get_partial_object(int dev, uint32_t handle, const char *path, uint32
 usb_mtp_op_unknown(int dev, uint32_t code) "dev %d, command code 0x%x"
 usb_mtp_object_alloc(int dev, uint32_t handle, const char *path) "dev %d, handle 0x%x, path %s"
 usb_mtp_object_free(int dev, uint32_t handle, const char *path) "dev %d, handle 0x%x, path %s"
+usb_mtp_add_child(int dev, uint32_t handle, const char *path) "dev %d, handle 0x%x, path %s"
 
 # hw/usb/host-libusb.c
 usb_host_open_started(int bus, int addr) "dev %d:%d"