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é <berra...@redhat.com> > 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é <berra...@redhat.com> > > --- > > 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 :|