Re: [Qemu-devel] [PATCH v6 05/11] hw/usb: switch MTP to use new inotify APIs

[Daniel P . Berrangé]

On Wed, Nov 07, 2018 at 10:26:29PM +0400, Marc-André Lureau wrote:
> On Fri, Oct 19, 2018 at 5:42 PM Daniel P. Berrangé <address@hidden> wrote:
> >
> > The internal inotify APIs allow alot of conditional statements to be
> 
> a lot
> 
> > cleared out, and provide a simpler callback for handling events.
> >
> > Signed-off-by: Daniel P. Berrangé <address@hidden>
> > ---
> >  hw/usb/dev-mtp.c    | 250 ++++++++++++++++----------------------------
> >  hw/usb/trace-events |   2 +-
> >  2 files changed, 93 insertions(+), 159 deletions(-)
> >
> > diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> > index ccbe25820b..1a8d0f088d 100644
> > --- a/hw/usb/dev-mtp.c
> > +++ b/hw/usb/dev-mtp.c
> > @@ -15,13 +15,11 @@
> >  #include <dirent.h>
> >
> >  #include <sys/statvfs.h>
> > -#ifdef CONFIG_INOTIFY1
> > -#include <sys/inotify.h>
> > -#include "qemu/main-loop.h"
> > -#endif
> > +
> >
> >  #include "qemu-common.h"
> >  #include "qemu/iov.h"
> > +#include "qemu/filemonitor.h"
> >  #include "trace.h"
> >  #include "hw/usb.h"
> >  #include "desc.h"
> > @@ -124,7 +122,6 @@ enum {
> >      EP_EVENT,
> >  };
> >
> > -#ifdef CONFIG_INOTIFY1
> >  typedef struct MTPMonEntry MTPMonEntry;
> >
> >  struct MTPMonEntry {
> > @@ -133,7 +130,6 @@ struct MTPMonEntry {
> >
> >      QTAILQ_ENTRY(MTPMonEntry) next;
> >  };
> > -#endif
> >
> >  struct MTPControl {
> >      uint16_t     code;
> > @@ -162,10 +158,8 @@ struct MTPObject {
> >      char         *name;
> >      char         *path;
> >      struct stat  stat;
> > -#ifdef CONFIG_INOTIFY1
> > -    /* inotify watch cookie */
> > +    /* file monitor watch cookie */
> >      int          watchfd;
> 
> Why not rename it watchid to avoid confusion?

Yes, will do.

> > -static void inotify_watchfn(void *arg)
> > +static void file_monitor_event(int wd,
> > +                               QFileMonitorEvent ev,
> > +                               const char *name,
> > +                               void *opaque)
> >  {
> > -    MTPState *s = arg;
> > -    ssize_t bytes;
> > -    /* From the man page: atleast one event can be read */
> > -    int pos;
> > -    char buf[sizeof(struct inotify_event) + NAME_MAX + 1];
> > -
> > -    for (;;) {
> > -        bytes = read(s->inotifyfd, buf, sizeof(buf));
> > -        pos = 0;
> > -
> > -        if (bytes <= 0) {
> > -            /* Better luck next time */
> > +    MTPState *s = opaque;
> > +    int watchfd = 0;
> > +    MTPObject *parent = usb_mtp_object_lookup_wd(s, wd);
> > +    MTPMonEntry *entry = NULL;
> > +    MTPObject *o;
> > +
> > +    if (!parent) {
> > +        return;
> > +    }
> > +
> > +    switch (ev) {
> > +    case QFILE_MONITOR_EVENT_CREATED:
> > +        if (usb_mtp_object_lookup_name(parent, name, -1)) {
> > +            /* Duplicate create event */
> >              return;
> >          }
> > +        entry = g_new0(MTPMonEntry, 1);
> > +        entry->handle = s->next_handle;
> > +        entry->event = EVT_OBJ_ADDED;
> > +        o = usb_mtp_add_child(s, parent, name);
> > +        if (!o) {
> > +            g_free(entry);
> > +            return;
> > +        }
> > +        o->watchfd = watchfd;
> 
> this effectively always set o->watchfd to 0, which is already
> initialized to 0 with g_new0(), you can drop it

Yeah, pre-existing pointless code, so I've dropped it.


> >  static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
> >  {
> >      struct dirent *entry;
> >      DIR *dir;
> > +    Error *err = NULL;
> >
> >      if (o->have_children) {
> >          return;
> > @@ -662,16 +596,19 @@ static void usb_mtp_object_readdir(MTPState *s, 
> > MTPObject *o)
> >      if (!dir) {
> >          return;
> >      }
> > -#ifdef CONFIG_INOTIFY1
> > -    int watchfd = usb_mtp_add_watch(s->inotifyfd, o->path);
> > +
> > +    int watchfd = qemu_file_monitor_add_watch(s->file_monitor, o->path, 
> > NULL,
> > +                                              file_monitor_event, s, &err);
> 
> There is an add_watch(), but I don't see the corresponding
> remove_watch(). This may probably cause crashes if MTPState is freed.

Yes, I've added code to remove the watch, and also
to use a private file_monitor instance so free'ing
that will release all watches as a safety net.

> 
> >      if (watchfd == -1) {
> > -        fprintf(stderr, "usb-mtp: failed to add watch for %s\n", o->path);
> > +        fprintf(stderr, "usb-mtp: failed to add watch for %s: %s\n", 
> > o->path,
> > +                error_get_pretty(err));
> 
> maybe it's a good time to turn into error_report() ?

Yep


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|