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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Nov 07, 2018 at 10:08:05PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Oct 19, 2018 at 5:41 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > The inotify userspace API for reading events is quite horrible, so it is
> > useful to wrap it in a more friendly API to avoid duplicating code
> > across many users in QEMU. Wrapping it also allows introduction of a
> > platform portability layer, so that we can add impls for non-Linux based
> > equivalents in future.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---

> > +struct QFileMonitor {
> > +    QemuMutex lock;
> > +    int fd;
> > +
> > +    GHashTable *dirs; /* dirname => QFileMonitorDir */
> > +    GHashTable *idmap; /* inotify ID => dirname */
> > +};
> > +
> > +
> > +typedef struct {
> > +    int id; /* watch ID */
> > +    char *filename; /* optional filter */
> > +    QFileMonitorHandler cb;
> > +    void *opaque;
> > +} QFileMonitorWatch;
> > +
> > +
> > +typedef struct {
> > +    char *path;
> > +    int id; /* inotify ID */
> > +    int nextid; /* watch ID counter */
> > +    gsize nwatches;
> > +    QFileMonitorWatch *watches;
> > +} QFileMonitorDir;
> > +
> > +
> > +#ifdef CONFIG_INOTIFY1
> > +#include <sys/inotify.h>
> > +
> > +static void qemu_file_monitor_watch(void *arg)
> > +{
> > +    QFileMonitor *mon = arg;
> > +    char buf[4096]
> > +        __attribute__ ((aligned(__alignof__(struct inotify_event))));
> > +    int used = 0;
> > +    int len = read(mon->fd, buf, sizeof(buf));
> > +
> > +    qemu_mutex_lock(&mon->lock);
> 
> I suppose the lock should guard from mon->fd above, or there might be
> a race when modifying/removing a watch from a different thread.

The mutex is only there to protect the "dirs" and "idmap" hash tables.

The "fd" has enough safety - the kernel is fine with you reading
from the fd, while another thread is adding/removing files on
the inotify handle.

Since the QFileMonitor object is a singleton that can never be
free'd we'll never race with closing 'fd either.

> 
> > +
> > +    if (len < 0) {
> > +        if (errno != EAGAIN) {
> > +            error_report("Failure monitoring inotify FD, disabling events");
> 
> strerror(errno) could be useful

Yep.


> > +static void
> > +qemu_file_monitor_dir_free(void *data)
> > +{
> > +    QFileMonitorDir *dir = data;
> > +
> > +    g_free(dir->watches);
> 
> for sake, I would add
> assert(dir->nwatches = 0)

Yep.


> > +#ifdef CONFIG_INOTIFY1
> > +int
> > +qemu_file_monitor_add_watch(QFileMonitor *mon,
> > +                            const char *dirpath,
> > +                            const char *filename,
> > +                            QFileMonitorHandler cb,
> > +                            void *opaque,
> > +                            Error **errp)
> > +{
> > +    QFileMonitorDir *dir;
> > +    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);
> > +
> > +        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;
> > +
> > +        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);
> > +        }
> > +    }
> > +
> > +    dir->watches = g_renew(QFileMonitorWatch, dir->watches, dir->nwatches + 1);
> 
> GArray could eventually make handling of watches a bit simpler
> (counting, resizing, removing etc)

ok, i'll have a look at that API.

> 
> > +
> > +    dir->watches[dir->nwatches].id = ++dir->nextid;
> > +    dir->watches[dir->nwatches].filename = filename ? g_strdup(filename) : NULL;
> 
> g_strdup(NULL) returns NULL already

Ah, I forget that.

> 
> > +    dir->watches[dir->nwatches].cb = cb;
> > +    dir->watches[dir->nwatches].opaque = opaque;
> > +    dir->nwatches++;
> > +
> > +    trace_qemu_file_monitor_add_watch(mon, dirpath,
> > +                                      filename ? filename : "<none>",
> > +                                      cb, opaque,
> > +                                      dir->watches[dir->nwatches - 1].id);
> > +
> > +    ret = 0;
> > +
> > + cleanup:
> > +    qemu_mutex_unlock(&mon->lock);
> > +    return ret;
> > +}
> > +
> > +
> > +void qemu_file_monitor_remove_watch(QFileMonitor *mon,
> > +                                    const char *dirpath,
> > +                                    int id)
> > +{
> > +    QFileMonitorDir *dir;
> > +    gsize i;
> > +
> > +    qemu_mutex_lock(&mon->lock);
> > +
> > +    trace_qemu_file_monitor_remove_watch(mon, dirpath, id);
> > +
> > +    dir = g_hash_table_lookup(mon->dirs, dirpath);
> > +    if (!dir) {
> > +        goto cleanup;
> > +    }
> > +
> > +    for (i = 0; i < dir->nwatches; i++) {
> > +        if (dir->watches[i].id == id) {
> > +            if (i < (dir->nwatches - 1)) {
> > +                memmove(dir->watches + i,
> > +                        dir->watches + i + 1,
> > +                        sizeof(QFileMonitorWatch) *
> > +                        (dir->nwatches - (i + 1)));
> > +                dir->watches = g_renew(QFileMonitorWatch, dir->watches,
> > +                                       dir->nwatches - 1);
> > +                dir->nwatches--;
> > +            }
> > +            break;
> > +        }
> > +    }
> > +
> > +    if (dir->nwatches == 0) {
> > +        inotify_rm_watch(mon->fd, dir->id);
> > +        trace_qemu_file_monitor_disable_watch(mon, dir->path, dir->id);
> > +
> > +        g_hash_table_remove(mon->idmap, GINT_TO_POINTER(dir->id));
> > +        g_hash_table_remove(mon->dirs, dir->path);
> > +    }
> > +
> > + cleanup:
> > +    qemu_mutex_lock(&mon->lock);
> > +}
> > +
> > +#else
> > +int
> > +qemu_file_monitor_add_watch(QFileMonitor *mon,
> > +                            const char *dirpath,
> > +                            const char *filename,
> > +                            QFileMonitorHandler cb,
> > +                            void *opaque,
> > +                            Error **errp)
> > +{
> > +    error_setg(errp, "File monitoring not available on this platform");
> > +    return -1;
> > +}
> > +
> > +void qemu_file_monitor_remove_watch(QFileMonitor *mon,
> > +                                    const char *dirpath,
> > +                                    int id)
> > +{
> > +}
> 
> Wouldn't it be cleaner with stubs/ ?

I guess we could do it that way.


> 
> Looks good, but would be even better with tests :)

I'll see what I can do about that.

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 :|