Message ID | 1446595225-23608-3-git-send-email-bsd@redhat.com |
---|---|
State | New |
Headers | show |
> +#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
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
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?
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
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
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.
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
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
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
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
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"
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(-)