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 :|
