Re: [Qemu-devel] [PATCH v4 01/17] util: add helper APIs for dealing with inotify in portable manner

[Daniel P . Berrangé]

On Wed, Mar 13, 2019 at 01:19:58PM -0400, Bandan Das wrote:
> Daniel P. Berrangé <address@hidden> writes:
> 
> > On Tue, Mar 12, 2019 at 07:07:42PM -0400, Bandan Das wrote:
> >> Daniel P. Berrangé <address@hidden> 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 :|