[Qemu-devel] [PATCH 0/3] usb-mtp events support

[Qemu-devel] [PATCH 0/3] usb-mtp events support

[Bandan Das]

This series adds support for mtp events that are piggybacked on
top of the Linux provided inotify mechanism. It performs well with some
light unit testing in a linux guest. The mtp share is still read only,
but now the guest will notice updates to the share as long as the
mtp client being used supports it.

Bandan Das (3):
  usb-mtp: use a list for keeping track of children
  usb-mtp: Add support for inotify based file monitoring
  usb-mtp: add support for basic mtp events

 hw/usb/dev-mtp.c | 339 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 trace-events     |   4 +
 2 files changed, 319 insertions(+), 24 deletions(-)

-- 
2.4.3

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

[Bandan Das]

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(-)

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"
-- 
2.4.3

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

[Gerd Hoffmann]

>  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

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

[Bandan Das]

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

[Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring

[Bandan Das]

For now, we use inotify watches to track only a small number of
events, namely, add, delete and modify. Note that for delete, the kernel
already deactivates the watch for us and we just need to
take care of modifying our internal state.

Suggested-by: Gerd Hoffman <kraxel@redhat.com>
Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 250 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 trace-events     |   3 +
 2 files changed, 251 insertions(+), 2 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 37dfa13..79d4ab0 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -15,9 +15,11 @@
 
 #include <sys/stat.h>
 #include <sys/statvfs.h>
+#include <sys/inotify.h>
 
 #include "qemu-common.h"
 #include "qemu/iov.h"
+#include "qemu/main-loop.h"
 #include "trace.h"
 #include "hw/usb.h"
 #include "hw/usb/desc.h"
@@ -77,6 +79,7 @@ typedef struct MTPState MTPState;
 typedef struct MTPControl MTPControl;
 typedef struct MTPData MTPData;
 typedef struct MTPObject MTPObject;
+typedef struct MTPMonEntry MTPMonEntry;
 
 enum {
     EP_DATA_IN = 1,
@@ -84,6 +87,19 @@ enum {
     EP_EVENT,
 };
 
+struct MTPMonEntry {
+    uint32_t event;
+    uint32_t handle;
+
+    QTAILQ_ENTRY(MTPMonEntry) next;
+};
+
+enum inotify_event_type {
+    CREATE = 1,
+    DELETE = 2,
+    MODIFY = 3,
+};
+
 struct MTPControl {
     uint16_t     code;
     uint32_t     trans;
@@ -108,6 +124,8 @@ struct MTPObject {
     char         *name;
     char         *path;
     struct stat  stat;
+    /* inotify watch cookie */
+    int          watchfd;
     MTPObject    *parent;
     uint32_t     nchildren;
     QLIST_HEAD(, MTPObject) children;
@@ -121,6 +139,8 @@ struct MTPState {
     char         *root;
     char         *desc;
     uint32_t     flags;
+    /* inotify descriptor */
+    int          inotifyfd;
 
     MTPData      *data_in;
     MTPData      *data_out;
@@ -129,6 +149,7 @@ struct MTPState {
     uint32_t     next_handle;
 
     QTAILQ_HEAD(, MTPObject) objects;
+    QTAILQ_HEAD(events, MTPMonEntry) events;
 };
 
 #define TYPE_USB_MTP "usb-mtp"
@@ -270,6 +291,40 @@ static const USBDesc desc = {
 };
 
 /* ----------------------------------------------------------------------- */
+static MTPObject *usb_mtp_object_lookup_name(MTPObject *parent,
+                                             char *name, int len)
+{
+    MTPObject *iter;
+
+    QLIST_FOREACH(iter, &parent->children, list) {
+        if (strncmp(iter->name, name, len) == 0) {
+            return iter;
+        }
+    }
+
+    return NULL;
+}
+
+static MTPObject *usb_mtp_object_lookup_wd(MTPState *s, int wd)
+{
+    MTPObject *iter;
+
+    QTAILQ_FOREACH(iter, &s->objects, next) {
+        if (iter->watchfd == wd) {
+            return iter;
+        }
+    }
+
+    return NULL;
+}
+
+static int usb_mtp_add_watch(int inotifyfd, char *path)
+{
+    uint32_t mask = IN_CREATE | IN_DELETE | IN_MODIFY |
+        IN_ISDIR;
+
+    return inotify_add_watch(inotifyfd, path, mask);
+}
 
 static MTPObject *usb_mtp_object_alloc(MTPState *s, uint32_t handle,
                                        MTPObject *parent, char *name)
@@ -316,6 +371,46 @@ ignore:
     return NULL;
 }
 
+static void usb_mtp_inotify_cleanup(MTPState *s)
+{
+    MTPMonEntry *e;
+
+    if (!s->inotifyfd) {
+        return;
+    }
+
+    qemu_set_fd_handler(s->inotifyfd, NULL, NULL, s);
+    close(s->inotifyfd);
+
+    QTAILQ_FOREACH(e, &s->events, next) {
+        QTAILQ_REMOVE(&s->events, e, next);
+        g_free(e);
+    }
+}
+
+static int usb_mtp_inotify_mon(MTPState *s, MTPObject *o)
+{
+    int watchfd;
+
+    if (!s->inotifyfd) {
+        return 0;
+    }
+    assert(o->format == FMT_ASSOCIATION);
+    if (o->watchfd > 0) {
+        /* already watching */
+        return 0;
+    }
+
+    watchfd = usb_mtp_add_watch(s->inotifyfd, o->path);
+    if (watchfd == -1) {
+        return 1;
+    }
+
+    o->watchfd = watchfd;
+    trace_usb_mtp_mon(s->dev.addr, o->path, watchfd);
+
+    return 0;
+}
 static void usb_mtp_object_free(MTPState *s, MTPObject *o)
 {
     MTPObject *iter;
@@ -370,6 +465,149 @@ static MTPObject *usb_mtp_add_child(MTPState *s, MTPObject *o,
     return child;
 }
 
+static void inotify_watchfn(void *arg)
+{
+    MTPState *s = arg;
+    ssize_t bytes;
+    int error = 0;
+    /* From the man page: atleast one event can be read */
+    int len = sizeof(struct inotify_event) + NAME_MAX + 1;
+    char buf[len];
+
+    for (;;) {
+        char *p;
+        bytes = read(s->inotifyfd, buf, len);
+
+        if (bytes <= 0) {
+            /* Better luck next time */
+            goto done;
+        }
+
+        /*
+         * TODO: Ignore initiator initiated events.
+         * For now we are good because the store is RO
+         */
+        for (p = buf; p < buf + bytes;) {
+            struct inotify_event *event = (struct inotify_event *)p;
+            int watchfd = 0;
+            uint32_t mask = event->mask & (IN_CREATE | IN_DELETE |
+                                           IN_MODIFY | IN_IGNORED);
+            MTPObject *parent = usb_mtp_object_lookup_wd(s, event->wd);
+            MTPMonEntry *entry = NULL;
+            MTPObject *o;
+            char *name, *path;
+
+            /*
+             * TODO: Complain even on the slightest hint that
+             * something has gone wrong. Eventually, it makes
+             * sense to process remaining events, if any.
+             */
+            if (!parent) {
+                error = 1;
+                goto done;
+            }
+
+            switch (mask) {
+            case IN_CREATE:
+                if (event->mask & IN_ISDIR) {
+                    /* Add a new watch asap so as to not lose events */
+                    name = g_strndup(event->name, event->len);
+                    path = g_strdup_printf("%s/%s", parent->path, name);
+
+                    watchfd = usb_mtp_add_watch(s->inotifyfd, path);
+                    g_free(path);
+                    g_free(name);
+
+                    if (watchfd == -1) {
+                        error = 1;
+                        goto done;
+                    }
+                }
+
+                entry = g_new0(MTPMonEntry, 1);
+                entry->handle = s->next_handle;
+                entry->event = CREATE;
+                o = usb_mtp_add_child(s, parent, event->name);
+                if (!o) {
+                    error = 1;
+                    g_free(entry);
+                    goto done;
+                }
+                o->watchfd = watchfd;
+                trace_usb_mtp_inotify_event(s->dev.addr, path,
+                                      event->mask, "Obj Added");
+                break;
+
+            case IN_DELETE:
+                /*
+                 * The kernel issues a IN_IGNORED event
+                 * when a dir containing a watchpoint is
+                 * deleted
+                 */
+                o = usb_mtp_object_lookup_name(parent, event->name, event->len);
+                if (!o) {
+                    error = 1;
+                    goto done;
+                }
+                entry = g_new0(MTPMonEntry, 1);
+                entry->handle = o->handle;
+                entry->event = DELETE;
+                usb_mtp_object_free(s, o);
+                trace_usb_mtp_inotify_event(s->dev.addr, o->path,
+                                      event->mask, "Obj Deleted");
+                break;
+
+            case IN_MODIFY:
+                o = usb_mtp_object_lookup_name(parent, event->name, event->len);
+                if (!o) {
+                    error = 1;
+                    goto done;
+                }
+                entry = g_new0(MTPMonEntry, 1);
+                entry->handle = o->handle;
+                entry->event = MODIFY;
+                trace_usb_mtp_inotify_event(s->dev.addr, o->path,
+                                      event->mask, "Obj Modified");
+                break;
+
+            case IN_IGNORED:
+                o = usb_mtp_object_lookup_name(parent, event->name, event->len);
+                trace_usb_mtp_inotify_event(s->dev.addr, o->path,
+                                      event->mask, "Obj ignored");
+                break;
+
+            default:
+                error = 1;
+                goto done;
+            }
+
+            if (entry) {
+                QTAILQ_INSERT_HEAD(&s->events, entry, next);
+            }
+            p += sizeof(struct inotify_event) + event->len;
+        }
+    }
+done:
+    if (error) {
+        fprintf(stderr, "usb-mtp: failed to parse inotify event\n");
+    }
+}
+
+static int usb_mtp_inotify_init(MTPState *s)
+{
+    int fd = inotify_init1(IN_NONBLOCK);
+    if (fd == -1) {
+        return 1;
+    }
+
+    QTAILQ_INIT(&s->events);
+    s->inotifyfd = fd;
+
+    qemu_set_fd_handler(fd, inotify_watchfn, NULL, s);
+
+    return 0;
+}
+
 static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
 {
     struct dirent *entry;
@@ -639,11 +877,11 @@ static MTPData *usb_mtp_get_object_handles(MTPState *s, MTPControl *c,
 {
     MTPData *d = usb_mtp_data_alloc(c);
     uint32_t i = 0, handles[o->nchildren];
-    MTPObject *iter;
+    MTPObject *iter, *next;
 
     trace_usb_mtp_op_get_object_handles(s->dev.addr, o->handle, o->path);
 
-    QLIST_FOREACH(iter, &o->children, list) {
+    QLIST_FOREACH_SAFE(iter, &o->children, list, next) {
         handles[i++] = iter->handle;
     }
     assert(i == o->nchildren);
@@ -777,11 +1015,15 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
         trace_usb_mtp_op_open_session(s->dev.addr);
         s->session = c->argv[0];
         usb_mtp_object_alloc(s, s->next_handle++, NULL, s->root);
+        if (usb_mtp_inotify_init(s)) {
+            fprintf(stderr, "usb-mtp: file monitoring init failed\n");
+        }
         break;
     case CMD_CLOSE_SESSION:
         trace_usb_mtp_op_close_session(s->dev.addr);
         s->session = 0;
         s->next_handle = 0;
+        usb_mtp_inotify_cleanup(s);
         usb_mtp_object_free(s, QTAILQ_FIRST(&s->objects));
         assert(QTAILQ_EMPTY(&s->objects));
         break;
@@ -827,6 +1069,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
             return;
         }
         usb_mtp_object_readdir(s, o);
+        if (usb_mtp_inotify_mon(s, o)) {
+            fprintf(stderr, "usb-mtp: adding watch for %s failed\n", o->path);
+        }
         if (c->code == CMD_GET_NUM_OBJECTS) {
             trace_usb_mtp_op_get_num_objects(s->dev.addr, o->handle, o->path);
             nres = 1;
@@ -907,6 +1152,7 @@ static void usb_mtp_handle_reset(USBDevice *dev)
 
     trace_usb_mtp_reset(s->dev.addr);
 
+    usb_mtp_inotify_cleanup(s);
     usb_mtp_object_free(s, QTAILQ_FIRST(&s->objects));
     s->session = 0;
     usb_mtp_data_free(s->data_in);
diff --git a/trace-events b/trace-events
index ba4473d..84c80fa 100644
--- a/trace-events
+++ b/trace-events
@@ -553,6 +553,9 @@ 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"
+usb_mtp_inotify_mon(int dev, const char *path, int fd) "dev %d, path %s watchfd %d"
+usb_mtp_inotify_event(int dev, const char *path, uint32_t mask, const char *s) "dev %d, path %s mask 0x%x event %s"
+usb_mtp_mon(int dev, const char *path, int watchfd) "dev %d path %s watchfd %d"
 
 # hw/usb/host-libusb.c
 usb_host_open_started(int bus, int addr) "dev %d:%d"
-- 
2.4.3

Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring

[Gerd Hoffmann]

> +#include <sys/inotify.h>

What happens on non-linux systems?

I guess we need some #ifdefs to not break the build there, or enable mtp
only for CONFIG_LINUX=y instead of CONFIG_POSIX=y.

> +enum inotify_event_type {
> +    CREATE = 1,
> +    DELETE = 2,
> +    MODIFY = 3,
> +};

Patch #3 removes this, I'd suggest to extent "enum mtp_code" with the
event codes here so we don't need this temporary thing.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring

[Bandan Das]

Gerd Hoffmann <kraxel@redhat.com> writes:

>> +#include <sys/inotify.h>
>
> What happens on non-linux systems?
>
> I guess we need some #ifdefs to not break the build there, or enable mtp
> only for CONFIG_LINUX=y instead of CONFIG_POSIX=y.

Oh, I had the ifdefs but somehow I confused myself by thinking CONFIG_POSIX
is enough not to compile on non-linux systems. I guess I was only thinking
about Windows. I will add them back.

>> +enum inotify_event_type {
>> +    CREATE = 1,
>> +    DELETE = 2,
>> +    MODIFY = 3,
>> +};
>
> Patch #3 removes this, I'd suggest to extent "enum mtp_code" with the
> event codes here so we don't need this temporary thing.

Ok.

> cheers,
>   Gerd

Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring

[Gerd Hoffmann]

On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote:
> +                    /* Add a new watch asap so as to not lose events
> */

This comment sounds like there is a race ("asap").  There isn't one,
correct ordering (adding the watch before reading the directory) is
enough to make sure you don't miss anything.  You might see create
events for objects already in the tree though, are you prepared to
handle that?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring

[Bandan Das]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote:
>> +                    /* Add a new watch asap so as to not lose events
>> */
>
> This comment sounds like there is a race ("asap").  There isn't one,
> correct ordering (adding the watch before reading the directory) is

Hmm, seems like there's still a small window. We may not have even
started processing the event because we are still processing the earlier
ones.

> enough to make sure you don't miss anything.  You might see create
> events for objects already in the tree though, are you prepared to
> handle that?

Oh, interesting.  Current version will happily add duplicate entries.
I will add a check.

> cheers,
>   Gerd

Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring

[Bandan Das]

Bandan Das <bsd@redhat.com> writes:

> Gerd Hoffmann <kraxel@redhat.com> writes:
>
>> On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote:
>>> +                    /* Add a new watch asap so as to not lose events
>>> */
>>
>> This comment sounds like there is a race ("asap").  There isn't one,
>> correct ordering (adding the watch before reading the directory) is
>
> Hmm, seems like there's still a small window. We may not have even
> started processing the event because we are still processing the earlier
> ones.
>
>> enough to make sure you don't miss anything.  You might see create
>> events for objects already in the tree though, are you prepared to
>> handle that?
>
> Oh, interesting.  Current version will happily add duplicate entries.
> I will add a check.

By the way, did you mean this as a duplicate create event ?
I took a quick look at fs/notify/inotify_fsnotify.c:

int inotify_handle_event(...
    ret = fsnotify_add_event(group, fsn_event, inotify_merge);                                                                                                              
        if (ret) {
                /* Our event wasn't used in the end. Free it. */
                fsnotify_destroy_event(group, fsn_event);
        }

So, atleast for consecutive duplicate events, the kernel seems to be doing
some filtering of its own.

>> cheers,
>>   Gerd

Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring

[Gerd Hoffmann]

On Mo, 2015-11-09 at 18:12 -0500, Bandan Das wrote:
> Gerd Hoffmann <kraxel@redhat.com> writes:
> 
> > On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote:
> >> +                    /* Add a new watch asap so as to not lose events
> >> */
> >
> > This comment sounds like there is a race ("asap").  There isn't one,
> > correct ordering (adding the watch before reading the directory) is
> 
> Hmm, seems like there's still a small window. We may not have even
> started processing the event because we are still processing the earlier
> ones.

> > enough to make sure you don't miss anything.  You might see create
> > events for objects already in the tree though, are you prepared to
> > handle that?
> 
> Oh, interesting.  Current version will happily add duplicate entries.
> I will add a check.

I think we are talking about the same thing here.
Things can run in parallel, like this:

    process copying a file tree | qemu with usb-mtp
    ----------------------------+--------------------------
    create directory            |
                                | inotify event #1 queued (dir)
                                | qemu fetches event #1
                                | qemu adds new inotify watch
    copy file into new dir      |
                                | inotify event #2 queued (file)
                                | qemu reads new directory
                                | qemu finds the new file
                                | qemu fetches event #2

So, yes, the kernel can add new inotify events for the new watch before
qemu finished processing the old event (especially before you are done
reading the directory), and if you are hitting that the effect is that
you see a create event for the new file even though you already have it
in the tree.

But it is impossible that you miss the creation of the new file (this is
what I meant with "there is no race").

hope this clarifies,
  Gerd

Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring

[Bandan Das]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On Mo, 2015-11-09 at 18:12 -0500, Bandan Das wrote:
>> Gerd Hoffmann <kraxel@redhat.com> writes:
>> 
>> > On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote:
>> >> +                    /* Add a new watch asap so as to not lose events
>> >> */
>> >
>> > This comment sounds like there is a race ("asap").  There isn't one,
>> > correct ordering (adding the watch before reading the directory) is
>> 
>> Hmm, seems like there's still a small window. We may not have even
>> started processing the event because we are still processing the earlier
>> ones.
>
>> > enough to make sure you don't miss anything.  You might see create
>> > events for objects already in the tree though, are you prepared to
>> > handle that?
>> 
>> Oh, interesting.  Current version will happily add duplicate entries.
>> I will add a check.
>
> I think we are talking about the same thing here.
> Things can run in parallel, like this:
>
>     process copying a file tree | qemu with usb-mtp
>     ----------------------------+--------------------------
>     create directory            |
>                                 | inotify event #1 queued (dir)
>                                 | qemu fetches event #1
>                                 | qemu adds new inotify watch
>     copy file into new dir      |
>                                 | inotify event #2 queued (file)
>                                 | qemu reads new directory
>                                 | qemu finds the new file
>                                 | qemu fetches event #2
>
> So, yes, the kernel can add new inotify events for the new watch before

Maybe I am missing something but what if the watch on dir was
added by qemu _after_ the file (say file1) was copied to it.
Then, the kernel would generate events for file2, file3 and so on but
never a CREATE event for file1. Isn't that a possibility ? So, what I mean
by that comment is that add a watchpoint soon enough but it could be
possible that by the time the watch is added, a few files might have already
been copied and will not generate events.

> qemu finished processing the old event (especially before you are done
> reading the directory), and if you are hitting that the effect is that
> you see a create event for the new file even though you already have it
> in the tree.
>
> But it is impossible that you miss the creation of the new file (this is
> what I meant with "there is no race").
>
> hope this clarifies,
>   Gerd

Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring

[Gerd Hoffmann]

  Hi,

> Maybe I am missing something but what if the watch on dir was
> added by qemu _after_ the file (say file1) was copied to it.
> Then, the kernel would generate events for file2, file3 and so on but
> never a CREATE event for file1. Isn't that a possibility ?

Yes.

> So, what I mean
> by that comment is that add a watchpoint soon enough but it could be
> possible that by the time the watch is added, a few files might have already
> been copied and will not generate events.

The readdir (in usb_mtp_object_readdir) will find them then.

While looking at the code:  I think there is no need to add the watch
right away.  We only read the directory when the guest actually requests
it.  Watching the directories we actually have scanned due to the guest
requesting it should be enough.

So I think adding the inotify watch in usb_mtp_object_readdir should
work just fine.  And the watch should be added before readdir to make
sure we don't miss anything.

File system events then can happen at three points in time:  (a) before
we add the watch, (b) after we add the watch, and before readdir, and
(c) after readdir.

   (a) we will see these files in the readdir call.
   (b) we will see these files in the readdir call
       *and* receive inotify events for them.
   (c) we will only get events for them.

The (b) events look bogous at first glance (create events for files you
already have in the list, but also delete events for files you don't
have in your list).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring

[Gerd Hoffmann]

On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote:
> +            case IN_DELETE:
> +                /*
> +                 * The kernel issues a IN_IGNORED event
> +                 * when a dir containing a watchpoint is
> +                 * deleted
> +                 */

Wrong place for the comment?

Re: [Qemu-devel] [PATCH 2/3] usb-mtp: Add support for inotify based file monitoring

[Bandan Das]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote:
>> +            case IN_DELETE:
>> +                /*
>> +                 * The kernel issues a IN_IGNORED event
>> +                 * when a dir containing a watchpoint is
>> +                 * deleted
>> +                 */
>
> Wrong place for the comment?

I added this to avoid confusion as to why are we not deleting the watch
during a delete event. I will reword the comment.

[Qemu-devel] [PATCH 3/3] usb-mtp: add support for basic mtp events

[Bandan Das]

When the host polls for events, we check our
events qlist and send one event at a time. Also, note
that the event packet needs to be sent in one go, so
I increased the max packet size to 64.

Tested with a linux guest.

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

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 79d4ab0..ad33ee8 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -64,6 +64,11 @@ enum mtp_code {
     /* format codes */
     FMT_UNDEFINED_OBJECT           = 0x3000,
     FMT_ASSOCIATION                = 0x3001,
+
+    /* event codes */
+    EVT_OBJ_ADDED                  = 0x4002,
+    EVT_OBJ_REMOVED                = 0x4003,
+    EVT_OBJ_INFO_CHANGED           = 0x4007,
 };
 
 typedef struct {
@@ -94,12 +99,6 @@ struct MTPMonEntry {
     QTAILQ_ENTRY(MTPMonEntry) next;
 };
 
-enum inotify_event_type {
-    CREATE = 1,
-    DELETE = 2,
-    MODIFY = 3,
-};
-
 struct MTPControl {
     uint16_t     code;
     uint32_t     trans;
@@ -205,7 +204,7 @@ static const USBDescIface desc_iface_full = {
         },{
             .bEndpointAddress      = USB_DIR_IN | EP_EVENT,
             .bmAttributes          = USB_ENDPOINT_XFER_INT,
-            .wMaxPacketSize        = 8,
+            .wMaxPacketSize        = 64,
             .bInterval             = 0x0a,
         },
     }
@@ -247,7 +246,7 @@ static const USBDescIface desc_iface_high = {
         },{
             .bEndpointAddress      = USB_DIR_IN | EP_EVENT,
             .bmAttributes          = USB_ENDPOINT_XFER_INT,
-            .wMaxPacketSize        = 8,
+            .wMaxPacketSize        = 64,
             .bInterval             = 0x0a,
         },
     }
@@ -526,7 +525,7 @@ static void inotify_watchfn(void *arg)
 
                 entry = g_new0(MTPMonEntry, 1);
                 entry->handle = s->next_handle;
-                entry->event = CREATE;
+                entry->event = EVT_OBJ_ADDED;
                 o = usb_mtp_add_child(s, parent, event->name);
                 if (!o) {
                     error = 1;
@@ -551,7 +550,7 @@ static void inotify_watchfn(void *arg)
                 }
                 entry = g_new0(MTPMonEntry, 1);
                 entry->handle = o->handle;
-                entry->event = DELETE;
+                entry->event = EVT_OBJ_REMOVED;
                 usb_mtp_object_free(s, o);
                 trace_usb_mtp_inotify_event(s->dev.addr, o->path,
                                       event->mask, "Obj Deleted");
@@ -565,7 +564,7 @@ static void inotify_watchfn(void *arg)
                 }
                 entry = g_new0(MTPMonEntry, 1);
                 entry->handle = o->handle;
-                entry->event = MODIFY;
+                entry->event = EVT_OBJ_INFO_CHANGED;
                 trace_usb_mtp_inotify_event(s->dev.addr, o->path,
                                       event->mask, "Obj Modified");
                 break;
@@ -1313,6 +1312,29 @@ static void usb_mtp_handle_data(USBDevice *dev, USBPacket *p)
         }
         break;
     case EP_EVENT:
+        if (!QTAILQ_EMPTY(&s->events)) {
+            struct MTPMonEntry *e = QTAILQ_LAST(&s->events, events);
+            uint32_t handle;
+            int len = sizeof(container) + sizeof(uint32_t);
+
+            if (p->iov.size < len) {
+                trace_usb_mtp_stall(s->dev.addr,
+                                    "packet too small to send event");
+                p->status = USB_RET_STALL;
+                return;
+            }
+
+            QTAILQ_REMOVE(&s->events, e, next);
+            container.length = cpu_to_le32(len);
+            container.type = cpu_to_le32(TYPE_EVENT);
+            container.code = cpu_to_le16(e->event);
+            container.trans = 0; /* no trans specific events */
+            handle = cpu_to_le32(e->handle);
+            usb_packet_copy(p, &container, sizeof(container));
+            usb_packet_copy(p, &handle, sizeof(uint32_t));
+            g_free(e);
+            return;
+        }
         p->status = USB_RET_NAK;
         return;
     default:
-- 
2.4.3

Re: [Qemu-devel] [PATCH 3/3] usb-mtp: add support for basic mtp events

[Gerd Hoffmann]

On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote:
> -            .wMaxPacketSize        = 8,
> +            .wMaxPacketSize        = 64,

This is a guest-visible change, so strictly speaking this needs be tied
to the machine type.  I think we can be a bit sloppy here though as
usb-mtp does not support live migration.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/3] usb-mtp: add support for basic mtp events

[Bandan Das]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On Di, 2015-11-03 at 19:00 -0500, Bandan Das wrote:
>> -            .wMaxPacketSize        = 8,
>> +            .wMaxPacketSize        = 64,
>
> This is a guest-visible change, so strictly speaking this needs be tied
> to the machine type.  I think we can be a bit sloppy here though as
> usb-mtp does not support live migration.

Makes sense. I will add a comment above then.

> cheers,
>   Gerd