qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Bandan Das
Subject: Re: [Qemu-devel] [PATCH v4 01/17] util: add helper APIs for dealing with inotify in portable manner
Date: Wed, 13 Mar 2019 13:19:58 -0400
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)

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.

Bandan

>
> Regards,
> Daniel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]