[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 05/11] hw/usb: switch MTP to use new inotify
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v8 05/11] hw/usb: switch MTP to use new inotify APIs |
Date: |
Sat, 16 Feb 2019 12:28:10 +0100 |
Hi
On Fri, Feb 15, 2019 at 4:57 PM Daniel P. Berrangé <address@hidden> wrote:
>
> The internal inotify APIs allow a lot of conditional statements to be
> cleared out, and provide a simpler callback for handling events.
>
> Signed-off-by: Daniel P. Berrangé <address@hidden>
Reviewed-by: Marc-André Lureau <address@hidden>
> ---
> hw/usb/dev-mtp.c | 274 ++++++++++++++++++--------------------------
> hw/usb/trace-events | 2 +-
> 2 files changed, 111 insertions(+), 165 deletions(-)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 169ee50c02..4ee4fc5a89 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -11,17 +11,16 @@
>
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> +#include "qemu/error-report.h"
> #include <wchar.h>
> #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"
> @@ -132,7 +131,6 @@ enum {
> EP_EVENT,
> };
>
> -#ifdef CONFIG_INOTIFY1
> typedef struct MTPMonEntry MTPMonEntry;
>
> struct MTPMonEntry {
> @@ -141,7 +139,6 @@ struct MTPMonEntry {
>
> QTAILQ_ENTRY(MTPMonEntry) next;
> };
> -#endif
>
> struct MTPControl {
> uint16_t code;
> @@ -172,10 +169,8 @@ struct MTPObject {
> char *name;
> char *path;
> struct stat stat;
> -#ifdef CONFIG_INOTIFY1
> - /* inotify watch cookie */
> - int watchfd;
> -#endif
> + /* file monitor watch id */
> + int watchid;
> MTPObject *parent;
> uint32_t nchildren;
> QLIST_HEAD(, MTPObject) children;
> @@ -198,11 +193,8 @@ struct MTPState {
> bool readonly;
>
> QTAILQ_HEAD(, MTPObject) objects;
> -#ifdef CONFIG_INOTIFY1
> - /* inotify descriptor */
> - int inotifyfd;
> + QFileMonitor *file_monitor;
> QTAILQ_HEAD(, MTPMonEntry) events;
> -#endif
> /* Responder is expecting a write operation */
> bool write_pending;
> struct {
> @@ -391,6 +383,7 @@ static MTPObject *usb_mtp_object_alloc(MTPState *s,
> uint32_t handle,
> goto ignore;
> }
>
> + o->watchid = -1;
> o->handle = handle;
> o->parent = parent;
> o->name = g_strdup(name);
> @@ -437,6 +430,10 @@ static void usb_mtp_object_free(MTPState *s, MTPObject
> *o)
>
> trace_usb_mtp_object_free(s->dev.addr, o->handle, o->path);
>
> + if (o->watchid != -1 && s->file_monitor) {
> + qemu_file_monitor_remove_watch(s->file_monitor, o->path, o->watchid);
> + }
> +
> QTAILQ_REMOVE(&s->objects, o, next);
> if (o->parent) {
> QLIST_REMOVE(o, list);
> @@ -488,6 +485,10 @@ static MTPObject *usb_mtp_object_lookup_name(MTPObject
> *parent,
> {
> MTPObject *iter;
>
> + if (len == -1) {
> + len = strlen(name);
> + }
> +
> QLIST_FOREACH(iter, &parent->children, list) {
> if (strncmp(iter->name, name, len) == 0) {
> return iter;
> @@ -497,13 +498,12 @@ static MTPObject *usb_mtp_object_lookup_name(MTPObject
> *parent,
> return NULL;
> }
>
> -#ifdef CONFIG_INOTIFY1
> -static MTPObject *usb_mtp_object_lookup_wd(MTPState *s, int wd)
> +static MTPObject *usb_mtp_object_lookup_id(MTPState *s, int id)
> {
> MTPObject *iter;
>
> QTAILQ_FOREACH(iter, &s->objects, next) {
> - if (iter->watchfd == wd) {
> + if (iter->watchid == id) {
> return iter;
> }
> }
> @@ -511,159 +511,103 @@ static MTPObject *usb_mtp_object_lookup_wd(MTPState
> *s, int wd)
> return NULL;
> }
>
> -static void inotify_watchfn(void *arg)
> +static void file_monitor_event(int id,
> + 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;
> + MTPObject *parent = usb_mtp_object_lookup_id(s, id);
> + 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;
> + }
> + trace_usb_mtp_file_monitor_event(s->dev.addr, name, "Obj Added");
> + break;
>
> + case QFILE_MONITOR_EVENT_DELETED:
> /*
> - * TODO: Ignore initiator initiated events.
> - * For now we are good because the store is RO
> + * The kernel issues a IN_IGNORED event
> + * when a dir containing a watchpoint is
> + * deleted, so we don't have to delete the
> + * watchpoint
> */
> - while (bytes > 0) {
> - char *p = buf + pos;
> - 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;
> -
> - pos = pos + sizeof(struct inotify_event) + event->len;
> - bytes = bytes - pos;
> -
> - if (!parent) {
> - continue;
> - }
> -
> - switch (mask) {
> - case IN_CREATE:
> - if (usb_mtp_object_lookup_name
> - (parent, event->name, event->len)) {
> - /* Duplicate create event */
> - continue;
> - }
> - entry = g_new0(MTPMonEntry, 1);
> - entry->handle = s->next_handle;
> - entry->event = EVT_OBJ_ADDED;
> - o = usb_mtp_add_child(s, parent, event->name);
> - if (!o) {
> - g_free(entry);
> - continue;
> - }
> - o->watchfd = watchfd;
> - trace_usb_mtp_inotify_event(s->dev.addr, event->name,
> - event->mask, "Obj Added");
> - break;
> -
> - case IN_DELETE:
> - /*
> - * The kernel issues a IN_IGNORED event
> - * when a dir containing a watchpoint is
> - * deleted, so we don't have to delete the
> - * watchpoint
> - */
> - o = usb_mtp_object_lookup_name(parent, event->name,
> event->len);
> - if (!o) {
> - continue;
> - }
> - entry = g_new0(MTPMonEntry, 1);
> - entry->handle = o->handle;
> - entry->event = EVT_OBJ_REMOVED;
> - trace_usb_mtp_inotify_event(s->dev.addr, o->path,
> - event->mask, "Obj Deleted");
> - usb_mtp_object_free(s, o);
> - break;
> -
> - case IN_MODIFY:
> - o = usb_mtp_object_lookup_name(parent, event->name,
> event->len);
> - if (!o) {
> - continue;
> - }
> - entry = g_new0(MTPMonEntry, 1);
> - entry->handle = o->handle;
> - entry->event = EVT_OBJ_INFO_CHANGED;
> - trace_usb_mtp_inotify_event(s->dev.addr, o->path,
> - event->mask, "Obj Modified");
> - break;
> -
> - case IN_IGNORED:
> - trace_usb_mtp_inotify_event(s->dev.addr, parent->path,
> - event->mask, "Obj parent dir ignored");
> - break;
> -
> - default:
> - fprintf(stderr, "usb-mtp: failed to parse inotify event\n");
> - continue;
> - }
> -
> - if (entry) {
> - QTAILQ_INSERT_HEAD(&s->events, entry, next);
> - }
> + o = usb_mtp_object_lookup_name(parent, name, -1);
> + if (!o) {
> + return;
> }
> - }
> -}
> + entry = g_new0(MTPMonEntry, 1);
> + entry->handle = o->handle;
> + entry->event = EVT_OBJ_REMOVED;
> + trace_usb_mtp_file_monitor_event(s->dev.addr, o->path, "Obj
> Deleted");
> + usb_mtp_object_free(s, o);
> + break;
>
> -static int usb_mtp_inotify_init(MTPState *s)
> -{
> - int fd;
> + case QFILE_MONITOR_EVENT_MODIFIED:
> + o = usb_mtp_object_lookup_name(parent, name, -1);
> + if (!o) {
> + return;
> + }
> + entry = g_new0(MTPMonEntry, 1);
> + entry->handle = o->handle;
> + entry->event = EVT_OBJ_INFO_CHANGED;
> + trace_usb_mtp_file_monitor_event(s->dev.addr, o->path, "Obj
> Modified");
> + break;
>
> - fd = inotify_init1(IN_NONBLOCK);
> - if (fd == -1) {
> - return 1;
> - }
> + case QFILE_MONITOR_EVENT_IGNORED:
> + trace_usb_mtp_file_monitor_event(s->dev.addr, parent->path,
> + "Obj parent dir ignored");
> + break;
>
> - QTAILQ_INIT(&s->events);
> - s->inotifyfd = fd;
> + case QFILE_MONITOR_EVENT_ATTRIBUTES:
> + break;
>
> - qemu_set_fd_handler(fd, inotify_watchfn, NULL, s);
> + default:
> + g_assert_not_reached();
> + }
>
> - return 0;
> + if (entry) {
> + QTAILQ_INSERT_HEAD(&s->events, entry, next);
> + }
> }
>
> -static void usb_mtp_inotify_cleanup(MTPState *s)
> +static void usb_mtp_file_monitor_cleanup(MTPState *s)
> {
> MTPMonEntry *e, *p;
>
> - if (!s->inotifyfd) {
> - return;
> - }
> -
> - qemu_set_fd_handler(s->inotifyfd, NULL, NULL, s);
> - close(s->inotifyfd);
> -
> QTAILQ_FOREACH_SAFE(e, &s->events, next, p) {
> QTAILQ_REMOVE(&s->events, e, next);
> g_free(e);
> }
> -}
>
> -static int usb_mtp_add_watch(int inotifyfd, const char *path)
> -{
> - uint32_t mask = IN_CREATE | IN_DELETE | IN_MODIFY;
> -
> - return inotify_add_watch(inotifyfd, path, mask);
> + qemu_file_monitor_free(s->file_monitor);
> + s->file_monitor = NULL;
> }
> -#endif
> +
>
> static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
> {
> struct dirent *entry;
> DIR *dir;
> int fd;
> + Error *err = NULL;
>
> if (o->have_children) {
> return;
> @@ -679,16 +623,21 @@ static void usb_mtp_object_readdir(MTPState *s,
> MTPObject *o)
> close(fd);
> return;
> }
> -#ifdef CONFIG_INOTIFY1
> - int watchfd = usb_mtp_add_watch(s->inotifyfd, o->path);
> - if (watchfd == -1) {
> - fprintf(stderr, "usb-mtp: failed to add watch for %s\n", o->path);
> - } else {
> - trace_usb_mtp_inotify_event(s->dev.addr, o->path,
> - 0, "Watch Added");
> - o->watchfd = watchfd;
> +
> + if (s->file_monitor) {
> + int id = qemu_file_monitor_add_watch(s->file_monitor, o->path, NULL,
> + file_monitor_event, s, &err);
> + if (id == -1) {
> + error_report("usb-mtp: failed to add watch for %s: %s", o->path,
> + error_get_pretty(err));
> + error_free(err);
> + } else {
> + trace_usb_mtp_file_monitor_event(s->dev.addr, o->path,
> + "Watch Added");
> + o->watchid = id;
> + }
> }
> -#endif
> +
> while ((entry = readdir(dir)) != NULL) {
> usb_mtp_add_child(s, o, entry->d_name);
> }
> @@ -1196,13 +1145,11 @@ enum {
> /* Assumes that children, if any, have been already freed */
> static void usb_mtp_object_free_one(MTPState *s, MTPObject *o)
> {
> -#ifndef CONFIG_INOTIFY1
> assert(o->nchildren == 0);
> QTAILQ_REMOVE(&s->objects, o, next);
> g_free(o->name);
> g_free(o->path);
> g_free(o);
> -#endif
> }
>
> static int usb_mtp_deletefn(MTPState *s, MTPObject *o, uint32_t trans)
> @@ -1301,6 +1248,7 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
> MTPData *data_in = NULL;
> MTPObject *o = NULL;
> uint32_t nres = 0, res0 = 0;
> + Error *err = NULL;
>
> /* sanity checks */
> if (c->code >= CMD_CLOSE_SESSION && s->session == 0) {
> @@ -1328,19 +1276,21 @@ 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);
> -#ifdef CONFIG_INOTIFY1
> - if (usb_mtp_inotify_init(s)) {
> - fprintf(stderr, "usb-mtp: file monitoring init failed\n");
> +
> + s->file_monitor = qemu_file_monitor_new(&err);
> + if (err) {
> + error_report("usb-mtp: file monitoring init failed: %s",
> + error_get_pretty(err));
> + error_free(err);
> + } else {
> + QTAILQ_INIT(&s->events);
> }
> -#endif
> break;
> case CMD_CLOSE_SESSION:
> trace_usb_mtp_op_close_session(s->dev.addr);
> s->session = 0;
> s->next_handle = 0;
> -#ifdef CONFIG_INOTIFY1
> - usb_mtp_inotify_cleanup(s);
> -#endif
> + usb_mtp_file_monitor_cleanup(s);
> usb_mtp_object_free(s, QTAILQ_FIRST(&s->objects));
> assert(QTAILQ_EMPTY(&s->objects));
> break;
> @@ -1553,9 +1503,7 @@ static void usb_mtp_handle_reset(USBDevice *dev)
>
> trace_usb_mtp_reset(s->dev.addr);
>
> -#ifdef CONFIG_INOTIFY1
> - usb_mtp_inotify_cleanup(s);
> -#endif
> + usb_mtp_file_monitor_cleanup(s);
> usb_mtp_object_free(s, QTAILQ_FIRST(&s->objects));
> s->session = 0;
> usb_mtp_data_free(s->data_in);
> @@ -2026,7 +1974,6 @@ static void usb_mtp_handle_data(USBDevice *dev,
> USBPacket *p)
> }
> break;
> case EP_EVENT:
> -#ifdef CONFIG_INOTIFY1
> if (!QTAILQ_EMPTY(&s->events)) {
> struct MTPMonEntry *e = QTAILQ_LAST(&s->events);
> uint32_t handle;
> @@ -2050,7 +1997,6 @@ static void usb_mtp_handle_data(USBDevice *dev,
> USBPacket *p)
> g_free(e);
> return;
> }
> -#endif
> p->status = USB_RET_NAK;
> return;
> default:
> diff --git a/hw/usb/trace-events b/hw/usb/trace-events
> index 2c18770ca5..99b1e8b8ce 100644
> --- a/hw/usb/trace-events
> +++ b/hw/usb/trace-events
> @@ -237,7 +237,7 @@ 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_event(int dev, const char *path, uint32_t mask, const char
> *s) "dev %d, path %s mask 0x%x event %s"
> +usb_mtp_file_monitor_event(int dev, const char *path, const char *s) "dev
> %d, path %s event %s"
>
> # hw/usb/host-libusb.c
> usb_host_open_started(int bus, int addr) "dev %d:%d"
> --
> 2.20.1
>
- [Qemu-devel] [PATCH v8 00/11] Add a standard authorization framework, Daniel P . Berrangé, 2019/02/15
- [Qemu-devel] [PATCH v8 04/11] hw/usb: fix const-ness for string params in MTP driver, Daniel P . Berrangé, 2019/02/15
- [Qemu-devel] [PATCH v8 03/11] hw/usb: don't set IN_ISDIR for inotify watch in MTP driver, Daniel P . Berrangé, 2019/02/15
- [Qemu-devel] [PATCH v8 02/11] qom: don't require user creatable objects to be registered, Daniel P . Berrangé, 2019/02/15
- [Qemu-devel] [PATCH v8 05/11] hw/usb: switch MTP to use new inotify APIs, Daniel P . Berrangé, 2019/02/15
- Re: [Qemu-devel] [PATCH v8 05/11] hw/usb: switch MTP to use new inotify APIs,
Marc-André Lureau <=
- [Qemu-devel] [PATCH v8 06/11] authz: add QAuthZ object as an authorization base class, Daniel P . Berrangé, 2019/02/15
- [Qemu-devel] [PATCH v8 07/11] authz: add QAuthZSimple object type for easy whitelist auth checks, Daniel P . Berrangé, 2019/02/15
- [Qemu-devel] [PATCH v8 01/11] util: add helper APIs for dealing with inotify in portable manner, Daniel P . Berrangé, 2019/02/15
- [Qemu-devel] [PATCH v8 08/11] authz: add QAuthZList object type for an access control list, Daniel P . Berrangé, 2019/02/15
- [Qemu-devel] [PATCH v8 09/11] authz: add QAuthZListFile object type for a file access control list, Daniel P . Berrangé, 2019/02/15
- [Qemu-devel] [PATCH v8 10/11] authz: add QAuthZPAM object type for authorizing using PAM, Daniel P . Berrangé, 2019/02/15