On Wed, Mar 13, 2019 at 01:19:58PM -0400, Bandan Das wrote: > Daniel P. Berrangé <[email protected]> writes: > > > On Tue, Mar 12, 2019 at 07:07:42PM -0400, Bandan Das wrote: > >> Daniel P. Berrangé <[email protected]> writes: > >> ... > >> > + > >> > +int > >> > +qemu_file_monitor_add_watch(QFileMonitor *mon, > >> > + const char *dirpath, > >> > + const char *filename, > >> > + QFileMonitorHandler cb, > >> > + void *opaque, > >> > + Error **errp) > >> > +{ > >> > + QFileMonitorDir *dir; > >> > + QFileMonitorWatch watch; > >> > + int ret = -1; > >> > + > >> > + qemu_mutex_lock(&mon->lock); > >> > + dir = g_hash_table_lookup(mon->dirs, dirpath); > >> > + if (!dir) { > >> > + int rv = inotify_add_watch(mon->fd, dirpath, > >> > + IN_CREATE | IN_DELETE | IN_MODIFY | > >> > + IN_MOVED_TO | IN_MOVED_FROM | > >> > IN_ATTRIB); > >> > + > >> > + if (rv < 0) { > >> > + error_setg_errno(errp, errno, "Unable to watch '%s'", > >> > dirpath); > >> > + goto cleanup; > >> > + } > >> > + > >> > + trace_qemu_file_monitor_enable_watch(mon, dirpath, rv); > >> > + > >> > + dir = g_new0(QFileMonitorDir, 1); > >> > + dir->path = g_strdup(dirpath); > >> > + dir->id = rv; > >> > + dir->watches = g_array_new(FALSE, TRUE, > >> > sizeof(QFileMonitorWatch)); > >> > + > >> > + g_hash_table_insert(mon->dirs, dir->path, dir); > >> > + g_hash_table_insert(mon->idmap, GINT_TO_POINTER(rv), dir); > >> > + > >> > + if (g_hash_table_size(mon->dirs) == 1) { > >> > + qemu_set_fd_handler(mon->fd, qemu_file_monitor_watch, NULL, > >> > mon); > >> > + } > >> > + } > >> > + > >> > + watch.id = dir->nextid++; > >> > + watch.filename = g_strdup(filename); > >> > + watch.cb = cb; > >> > + watch.opaque = opaque; > >> > + > >> > + g_array_append_val(dir->watches, watch); > >> > + > >> > + trace_qemu_file_monitor_add_watch(mon, dirpath, > >> > + filename ? filename : "<none>", > >> > + cb, opaque, watch.id); > >> > + > >> > + ret = watch.id; > >> > + > >> > >> This seems to break usb-mtp since we are returning watch.id. > >> Why are we not returning dir->id here ? usb-mtp relies on the watch > >> descriptor to handle events. > > > > dir->id is the low level inotify identifier. This is not supposed to be > > visible to any calling code, since that should instead be using the > > QEMU generated watch.id value. > > > > I guessed so even though I had the urge to just post a patch and return > dir->id :) > > > Can you give more info on how usb-mtp broke ? I tested USB MTP before > > sending this series and I saw it correctly detecting create / delete > > of files & reflecting this in the guest. > > > This is what I think is happening: > > Consider the mtproot to be /root/mtpshare and following directory hierarchy - > /root/mtpshare/dir1 > > Initiator: > User enters dir1 > Host: > usb_mtp_object_readdir is called which calls qemu_file_monitor_add_watch() > qemu_file_monitor_add_watch returns 0. > > Initiator: > User attempts to create a directory "dir2" in dir1. > Host: > The callback file_monitor_event() is invoked, id is 0. > file_monitor_event calls: > MTPObject *parent = usb_mtp_object_lookup_id(s, 0); > which returns the object associated with /root/mtpshare. > Further below, usb_mtp_add_child() will try to add the newly > created object but it will fail because it's looking for it > in /root/mtpshare(watchid=0) not /root/mtpshare/dir1. > > I believe the problem is that adding a watch point isn't returning > a unique identifier to identify the watch.
Yeah, I've found the problem and will send a fix tomorrow once I have unit tests for it. 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 :|
