Daniel P. Berrangé <berra...@redhat.com> writes:
> On Tue, Mar 12, 2019 at 07:07:42PM -0400, Bandan Das wrote:
>> Daniel P. Berrangé <berra...@redhat.com> 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.
Bandan
>
> Regards,
> Daniel
