[2/2] filemon: ensure watch IDs are unique to QFileMonitor scope
diff mbox series

Message ID 20190314151527.25533-3-berrange@redhat.com
State New
Headers show
Series
  • filemonitor: fix watch ID allocation for directories
Related show

Commit Message

Daniel P. Berrangé March 14, 2019, 3:15 p.m. UTC
The watch IDs are mistakenly only unique within the scope of the
directory being monitored. This is not useful for clients which are
monitoring multiple directories. They require watch IDs to be unique
globally within the QFileMonitor scope.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/test-util-filemonitor.c | 116 +++++++++++++++++++++++++++++++---
 util/filemonitor-inotify.c    |   5 +-
 2 files changed, 110 insertions(+), 11 deletions(-)

Comments

Marc-André Lureau March 14, 2019, 3:58 p.m. UTC | #1
On Thu, Mar 14, 2019 at 4:29 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The watch IDs are mistakenly only unique within the scope of the
> directory being monitored. This is not useful for clients which are
> monitoring multiple directories. They require watch IDs to be unique
> globally within the QFileMonitor scope.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  tests/test-util-filemonitor.c | 116 +++++++++++++++++++++++++++++++---
>  util/filemonitor-inotify.c    |   5 +-
>  2 files changed, 110 insertions(+), 11 deletions(-)
>
> diff --git a/tests/test-util-filemonitor.c b/tests/test-util-filemonitor.c
> index ea3715a8f4..71a7cf5de0 100644
> --- a/tests/test-util-filemonitor.c
> +++ b/tests/test-util-filemonitor.c
> @@ -35,6 +35,8 @@ enum {
>      QFILE_MONITOR_TEST_OP_RENAME,
>      QFILE_MONITOR_TEST_OP_TOUCH,
>      QFILE_MONITOR_TEST_OP_UNLINK,
> +    QFILE_MONITOR_TEST_OP_MKDIR,
> +    QFILE_MONITOR_TEST_OP_RMDIR,
>  };
>
>  typedef struct {
> @@ -298,6 +300,54 @@ test_file_monitor_events(void)
>            .eventid = QFILE_MONITOR_EVENT_DELETED },
>
>
> +        { .type = QFILE_MONITOR_TEST_OP_MKDIR,
> +          .filesrc = "fish", },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "fish", .watchid = 0,
> +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> +
> +
> +        { .type = QFILE_MONITOR_TEST_OP_ADD_WATCH,
> +          .filesrc = "fish/", .watchid = 4 },
> +        { .type = QFILE_MONITOR_TEST_OP_ADD_WATCH,
> +          .filesrc = "fish/one.txt", .watchid = 5 },
> +        { .type = QFILE_MONITOR_TEST_OP_CREATE,
> +          .filesrc = "fish/one.txt", },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "one.txt", .watchid = 4,
> +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "one.txt", .watchid = 5,
> +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> +
> +
> +        { .type = QFILE_MONITOR_TEST_OP_DEL_WATCH,
> +          .filesrc = "fish/one.txt", .watchid = 5 },
> +        { .type = QFILE_MONITOR_TEST_OP_RENAME,
> +          .filesrc = "fish/one.txt", .filedst = "two.txt", },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "one.txt", .watchid = 4,
> +          .eventid = QFILE_MONITOR_EVENT_DELETED },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "two.txt", .watchid = 0,
> +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "two.txt", .watchid = 2,
> +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> +
> +
> +        { .type = QFILE_MONITOR_TEST_OP_RMDIR,
> +          .filesrc = "fish", },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "", .watchid = 4,
> +          .eventid = QFILE_MONITOR_EVENT_IGNORED },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "fish", .watchid = 0,
> +          .eventid = QFILE_MONITOR_EVENT_DELETED },
> +        { .type = QFILE_MONITOR_TEST_OP_DEL_WATCH,
> +          .filesrc = "fish", .watchid = 4 },
> +
> +
>          { .type = QFILE_MONITOR_TEST_OP_UNLINK,
>            .filesrc = "two.txt", },
>          { .type = QFILE_MONITOR_TEST_OP_EVENT,
> @@ -366,6 +416,8 @@ test_file_monitor_events(void)
>          int fd;
>          int watchid;
>          struct utimbuf ubuf;
> +        char *watchdir;
> +        const char *watchfile;
>
>          pathsrc = g_strdup_printf("%s/%s", dir, op->filesrc);
>          if (op->filedst) {
> @@ -378,13 +430,26 @@ test_file_monitor_events(void)
>                  g_printerr("Add watch %s %s %d\n",
>                             dir, op->filesrc, op->watchid);
>              }
> +            if (op->filesrc && strchr(op->filesrc, '/')) {
> +                watchdir = g_strdup_printf("%s/%s", dir, op->filesrc);
> +                watchfile = strrchr(watchdir, '/');
> +                *(char *)watchfile = '\0';
> +                watchfile++;
> +                if (*watchfile == '\0') {
> +                    watchfile = NULL;
> +                }
> +            } else {
> +                watchdir = g_strdup(dir);
> +                watchfile = op->filesrc;
> +            }
>              watchid =
>                  qemu_file_monitor_add_watch(mon,
> -                                            dir,
> -                                            op->filesrc,
> +                                            watchdir,
> +                                            watchfile,
>                                              qemu_file_monitor_test_handler,
>                                              &data,
>                                              &local_err);
> +            g_free(watchdir);
>              if (watchid < 0) {
>                  g_printerr("Unable to add watch %s",
>                             error_get_pretty(local_err));
> @@ -400,9 +465,17 @@ test_file_monitor_events(void)
>              if (debug) {
>                  g_printerr("Del watch %s %d\n", dir, op->watchid);
>              }
> +            if (op->filesrc && strchr(op->filesrc, '/')) {
> +                watchdir = g_strdup_printf("%s/%s", dir, op->filesrc);
> +                watchfile = strrchr(watchdir, '/');
> +                *(char *)watchfile = '\0';
> +            } else {
> +                watchdir = g_strdup(dir);
> +            }
>              qemu_file_monitor_remove_watch(mon,
> -                                           dir,
> +                                           watchdir,
>                                             op->watchid);
> +            g_free(watchdir);
>              break;
>          case QFILE_MONITOR_TEST_OP_EVENT:
>              if (debug) {
> @@ -492,6 +565,28 @@ test_file_monitor_events(void)
>              }
>              break;
>
> +        case QFILE_MONITOR_TEST_OP_MKDIR:
> +            if (debug) {
> +                g_printerr("Mkdir %s\n", pathsrc);
> +            }
> +            if (mkdir(pathsrc, 0700) < 0) {
> +                g_printerr("Unable to mkdir %s: %s",
> +                           pathsrc, strerror(errno));
> +                goto cleanup;
> +            }
> +            break;
> +
> +        case QFILE_MONITOR_TEST_OP_RMDIR:
> +            if (debug) {
> +                g_printerr("Rmdir %s\n", pathsrc);
> +            }
> +            if (rmdir(pathsrc) < 0) {
> +                g_printerr("Unable to rmdir %s: %s",
> +                           pathsrc, strerror(errno));
> +                goto cleanup;
> +            }
> +            break;
> +
>          default:
>              g_assert_not_reached();
>          }
> @@ -532,13 +627,18 @@ test_file_monitor_events(void)
>              const QFileMonitorTestOp *op = &(ops[i]);
>              char *path = g_strdup_printf("%s/%s",
>                                           dir, op->filesrc);
> -            unlink(path);
> -            g_free(path);
> -            if (op->filedst) {
> -                path = g_strdup_printf("%s/%s",
> -                                       dir, op->filedst);
> +            if (op->type == QFILE_MONITOR_TEST_OP_MKDIR) {
> +                rmdir(path);
> +                g_free(path);
> +            } else {
>                  unlink(path);
>                  g_free(path);
> +                if (op->filedst) {
> +                    path = g_strdup_printf("%s/%s",
> +                                           dir, op->filedst);
> +                    unlink(path);
> +                    g_free(path);
> +                }
>              }
>          }
>          if (rmdir(dir) < 0) {
> diff --git a/util/filemonitor-inotify.c b/util/filemonitor-inotify.c
> index 3a72be037f..3eb29f860b 100644
> --- a/util/filemonitor-inotify.c
> +++ b/util/filemonitor-inotify.c
> @@ -29,7 +29,7 @@
>
>  struct QFileMonitor {
>      int fd;
> -
> +    int nextid; /* watch ID counter */
>      QemuMutex lock; /* protects dirs & idmap */
>      GHashTable *dirs; /* dirname => QFileMonitorDir */
>      GHashTable *idmap; /* inotify ID => dirname */
> @@ -47,7 +47,6 @@ typedef struct {
>  typedef struct {
>      char *path;
>      int id; /* inotify ID */
> -    int nextid; /* watch ID counter */
>      GArray *watches; /* QFileMonitorWatch elements */
>  } QFileMonitorDir;
>
> @@ -277,7 +276,7 @@ qemu_file_monitor_add_watch(QFileMonitor *mon,
>          }
>      }
>
> -    watch.id = dir->nextid++;
> +    watch.id = mon->nextid++;
>      watch.filename = g_strdup(filename);
>      watch.cb = cb;
>      watch.opaque = opaque;
> --
> 2.20.1
>
>
Bandan Das March 15, 2019, 4:27 p.m. UTC | #2
Daniel P. Berrangé <berrange@redhat.com> writes:

> The watch IDs are mistakenly only unique within the scope of the
> directory being monitored. This is not useful for clients which are
> monitoring multiple directories. They require watch IDs to be unique
> globally within the QFileMonitor scope.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tests/test-util-filemonitor.c | 116 +++++++++++++++++++++++++++++++---
>  util/filemonitor-inotify.c    |   5 +-
>  2 files changed, 110 insertions(+), 11 deletions(-)
>
> diff --git a/tests/test-util-filemonitor.c b/tests/test-util-filemonitor.c
> index ea3715a8f4..71a7cf5de0 100644
> --- a/tests/test-util-filemonitor.c
> +++ b/tests/test-util-filemonitor.c
> @@ -35,6 +35,8 @@ enum {
>      QFILE_MONITOR_TEST_OP_RENAME,
>      QFILE_MONITOR_TEST_OP_TOUCH,
>      QFILE_MONITOR_TEST_OP_UNLINK,
> +    QFILE_MONITOR_TEST_OP_MKDIR,
> +    QFILE_MONITOR_TEST_OP_RMDIR,
>  };
>  
>  typedef struct {
> @@ -298,6 +300,54 @@ test_file_monitor_events(void)
>            .eventid = QFILE_MONITOR_EVENT_DELETED },
>  
>  
> +        { .type = QFILE_MONITOR_TEST_OP_MKDIR,
> +          .filesrc = "fish", },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "fish", .watchid = 0,
> +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> +
> +
> +        { .type = QFILE_MONITOR_TEST_OP_ADD_WATCH,
> +          .filesrc = "fish/", .watchid = 4 },
> +        { .type = QFILE_MONITOR_TEST_OP_ADD_WATCH,
> +          .filesrc = "fish/one.txt", .watchid = 5 },
> +        { .type = QFILE_MONITOR_TEST_OP_CREATE,
> +          .filesrc = "fish/one.txt", },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "one.txt", .watchid = 4,
> +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "one.txt", .watchid = 5,
> +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> +
> +
> +        { .type = QFILE_MONITOR_TEST_OP_DEL_WATCH,
> +          .filesrc = "fish/one.txt", .watchid = 5 },
> +        { .type = QFILE_MONITOR_TEST_OP_RENAME,
> +          .filesrc = "fish/one.txt", .filedst = "two.txt", },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "one.txt", .watchid = 4,
> +          .eventid = QFILE_MONITOR_EVENT_DELETED },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "two.txt", .watchid = 0,
> +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "two.txt", .watchid = 2,
> +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> +
> +
> +        { .type = QFILE_MONITOR_TEST_OP_RMDIR,
> +          .filesrc = "fish", },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "", .watchid = 4,
> +          .eventid = QFILE_MONITOR_EVENT_IGNORED },
> +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> +          .filesrc = "fish", .watchid = 0,
> +          .eventid = QFILE_MONITOR_EVENT_DELETED },
> +        { .type = QFILE_MONITOR_TEST_OP_DEL_WATCH,
> +          .filesrc = "fish", .watchid = 4 },
> +
> +
>          { .type = QFILE_MONITOR_TEST_OP_UNLINK,
>            .filesrc = "two.txt", },
>          { .type = QFILE_MONITOR_TEST_OP_EVENT,
> @@ -366,6 +416,8 @@ test_file_monitor_events(void)
>          int fd;
>          int watchid;
>          struct utimbuf ubuf;
> +        char *watchdir;
> +        const char *watchfile;
>  
>          pathsrc = g_strdup_printf("%s/%s", dir, op->filesrc);
>          if (op->filedst) {
> @@ -378,13 +430,26 @@ test_file_monitor_events(void)
>                  g_printerr("Add watch %s %s %d\n",
>                             dir, op->filesrc, op->watchid);
>              }
> +            if (op->filesrc && strchr(op->filesrc, '/')) {
> +                watchdir = g_strdup_printf("%s/%s", dir, op->filesrc);
> +                watchfile = strrchr(watchdir, '/');
> +                *(char *)watchfile = '\0';
> +                watchfile++;
> +                if (*watchfile == '\0') {
> +                    watchfile = NULL;
> +                }
> +            } else {
> +                watchdir = g_strdup(dir);
> +                watchfile = op->filesrc;
> +            }
>              watchid =
>                  qemu_file_monitor_add_watch(mon,
> -                                            dir,
> -                                            op->filesrc,
> +                                            watchdir,
> +                                            watchfile,
>                                              qemu_file_monitor_test_handler,
>                                              &data,
>                                              &local_err);
> +            g_free(watchdir);
>              if (watchid < 0) {
>                  g_printerr("Unable to add watch %s",
>                             error_get_pretty(local_err));
> @@ -400,9 +465,17 @@ test_file_monitor_events(void)
>              if (debug) {
>                  g_printerr("Del watch %s %d\n", dir, op->watchid);
>              }
> +            if (op->filesrc && strchr(op->filesrc, '/')) {
> +                watchdir = g_strdup_printf("%s/%s", dir, op->filesrc);
> +                watchfile = strrchr(watchdir, '/');
> +                *(char *)watchfile = '\0';
> +            } else {
> +                watchdir = g_strdup(dir);
> +            }
>              qemu_file_monitor_remove_watch(mon,
> -                                           dir,
> +                                           watchdir,
>                                             op->watchid);
> +            g_free(watchdir);
>              break;
>          case QFILE_MONITOR_TEST_OP_EVENT:
>              if (debug) {
> @@ -492,6 +565,28 @@ test_file_monitor_events(void)
>              }
>              break;
>  
> +        case QFILE_MONITOR_TEST_OP_MKDIR:
> +            if (debug) {
> +                g_printerr("Mkdir %s\n", pathsrc);
> +            }
> +            if (mkdir(pathsrc, 0700) < 0) {
> +                g_printerr("Unable to mkdir %s: %s",
> +                           pathsrc, strerror(errno));
> +                goto cleanup;
> +            }
> +            break;
> +
> +        case QFILE_MONITOR_TEST_OP_RMDIR:
> +            if (debug) {
> +                g_printerr("Rmdir %s\n", pathsrc);
> +            }
> +            if (rmdir(pathsrc) < 0) {
> +                g_printerr("Unable to rmdir %s: %s",
> +                           pathsrc, strerror(errno));
> +                goto cleanup;
> +            }
> +            break;
> +
>          default:
>              g_assert_not_reached();
>          }
> @@ -532,13 +627,18 @@ test_file_monitor_events(void)
>              const QFileMonitorTestOp *op = &(ops[i]);
>              char *path = g_strdup_printf("%s/%s",
>                                           dir, op->filesrc);
> -            unlink(path);
> -            g_free(path);
> -            if (op->filedst) {
> -                path = g_strdup_printf("%s/%s",
> -                                       dir, op->filedst);
> +            if (op->type == QFILE_MONITOR_TEST_OP_MKDIR) {
> +                rmdir(path);
> +                g_free(path);
> +            } else {
>                  unlink(path);
>                  g_free(path);
> +                if (op->filedst) {
> +                    path = g_strdup_printf("%s/%s",
> +                                           dir, op->filedst);
> +                    unlink(path);
> +                    g_free(path);
> +                }
>              }
>          }
>          if (rmdir(dir) < 0) {
> diff --git a/util/filemonitor-inotify.c b/util/filemonitor-inotify.c
> index 3a72be037f..3eb29f860b 100644
> --- a/util/filemonitor-inotify.c
> +++ b/util/filemonitor-inotify.c
> @@ -29,7 +29,7 @@
>  
>  struct QFileMonitor {
>      int fd;
> -
> +    int nextid; /* watch ID counter */
>      QemuMutex lock; /* protects dirs & idmap */
>      GHashTable *dirs; /* dirname => QFileMonitorDir */
>      GHashTable *idmap; /* inotify ID => dirname */
> @@ -47,7 +47,6 @@ typedef struct {
>  typedef struct {
>      char *path;
>      int id; /* inotify ID */
> -    int nextid; /* watch ID counter */
>      GArray *watches; /* QFileMonitorWatch elements */
>  } QFileMonitorDir;
>  
> @@ -277,7 +276,7 @@ qemu_file_monitor_add_watch(QFileMonitor *mon,
>          }
>      }
>  
> -    watch.id = dir->nextid++;
> +    watch.id = mon->nextid++;
>      watch.filename = g_strdup(filename);
>      watch.cb = cb;
>      watch.opaque = opaque;

Thanks, this fixes it! I had a related question about the way
qemu_file_monitor_add_watch works.

Am I correct in understanding that if there is already a watch on a dir,
we return back mon->nextid++ i.e the next free id. Why don't we return
back the originally assigned watchid ?


Reviewed-and-tested-by: Bandan Das <bsd@redhat.com>
Daniel P. Berrangé March 15, 2019, 4:54 p.m. UTC | #3
On Fri, Mar 15, 2019 at 12:27:06PM -0400, Bandan Das wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > The watch IDs are mistakenly only unique within the scope of the
> > directory being monitored. This is not useful for clients which are
> > monitoring multiple directories. They require watch IDs to be unique
> > globally within the QFileMonitor scope.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  tests/test-util-filemonitor.c | 116 +++++++++++++++++++++++++++++++---
> >  util/filemonitor-inotify.c    |   5 +-
> >  2 files changed, 110 insertions(+), 11 deletions(-)
> >
> > diff --git a/tests/test-util-filemonitor.c b/tests/test-util-filemonitor.c
> > index ea3715a8f4..71a7cf5de0 100644
> > --- a/tests/test-util-filemonitor.c
> > +++ b/tests/test-util-filemonitor.c
> > @@ -35,6 +35,8 @@ enum {
> >      QFILE_MONITOR_TEST_OP_RENAME,
> >      QFILE_MONITOR_TEST_OP_TOUCH,
> >      QFILE_MONITOR_TEST_OP_UNLINK,
> > +    QFILE_MONITOR_TEST_OP_MKDIR,
> > +    QFILE_MONITOR_TEST_OP_RMDIR,
> >  };
> >  
> >  typedef struct {
> > @@ -298,6 +300,54 @@ test_file_monitor_events(void)
> >            .eventid = QFILE_MONITOR_EVENT_DELETED },
> >  
> >  
> > +        { .type = QFILE_MONITOR_TEST_OP_MKDIR,
> > +          .filesrc = "fish", },
> > +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> > +          .filesrc = "fish", .watchid = 0,
> > +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> > +
> > +
> > +        { .type = QFILE_MONITOR_TEST_OP_ADD_WATCH,
> > +          .filesrc = "fish/", .watchid = 4 },
> > +        { .type = QFILE_MONITOR_TEST_OP_ADD_WATCH,
> > +          .filesrc = "fish/one.txt", .watchid = 5 },
> > +        { .type = QFILE_MONITOR_TEST_OP_CREATE,
> > +          .filesrc = "fish/one.txt", },
> > +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> > +          .filesrc = "one.txt", .watchid = 4,
> > +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> > +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> > +          .filesrc = "one.txt", .watchid = 5,
> > +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> > +
> > +
> > +        { .type = QFILE_MONITOR_TEST_OP_DEL_WATCH,
> > +          .filesrc = "fish/one.txt", .watchid = 5 },
> > +        { .type = QFILE_MONITOR_TEST_OP_RENAME,
> > +          .filesrc = "fish/one.txt", .filedst = "two.txt", },
> > +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> > +          .filesrc = "one.txt", .watchid = 4,
> > +          .eventid = QFILE_MONITOR_EVENT_DELETED },
> > +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> > +          .filesrc = "two.txt", .watchid = 0,
> > +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> > +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> > +          .filesrc = "two.txt", .watchid = 2,
> > +          .eventid = QFILE_MONITOR_EVENT_CREATED },
> > +
> > +
> > +        { .type = QFILE_MONITOR_TEST_OP_RMDIR,
> > +          .filesrc = "fish", },
> > +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> > +          .filesrc = "", .watchid = 4,
> > +          .eventid = QFILE_MONITOR_EVENT_IGNORED },
> > +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> > +          .filesrc = "fish", .watchid = 0,
> > +          .eventid = QFILE_MONITOR_EVENT_DELETED },
> > +        { .type = QFILE_MONITOR_TEST_OP_DEL_WATCH,
> > +          .filesrc = "fish", .watchid = 4 },
> > +
> > +
> >          { .type = QFILE_MONITOR_TEST_OP_UNLINK,
> >            .filesrc = "two.txt", },
> >          { .type = QFILE_MONITOR_TEST_OP_EVENT,
> > @@ -366,6 +416,8 @@ test_file_monitor_events(void)
> >          int fd;
> >          int watchid;
> >          struct utimbuf ubuf;
> > +        char *watchdir;
> > +        const char *watchfile;
> >  
> >          pathsrc = g_strdup_printf("%s/%s", dir, op->filesrc);
> >          if (op->filedst) {
> > @@ -378,13 +430,26 @@ test_file_monitor_events(void)
> >                  g_printerr("Add watch %s %s %d\n",
> >                             dir, op->filesrc, op->watchid);
> >              }
> > +            if (op->filesrc && strchr(op->filesrc, '/')) {
> > +                watchdir = g_strdup_printf("%s/%s", dir, op->filesrc);
> > +                watchfile = strrchr(watchdir, '/');
> > +                *(char *)watchfile = '\0';
> > +                watchfile++;
> > +                if (*watchfile == '\0') {
> > +                    watchfile = NULL;
> > +                }
> > +            } else {
> > +                watchdir = g_strdup(dir);
> > +                watchfile = op->filesrc;
> > +            }
> >              watchid =
> >                  qemu_file_monitor_add_watch(mon,
> > -                                            dir,
> > -                                            op->filesrc,
> > +                                            watchdir,
> > +                                            watchfile,
> >                                              qemu_file_monitor_test_handler,
> >                                              &data,
> >                                              &local_err);
> > +            g_free(watchdir);
> >              if (watchid < 0) {
> >                  g_printerr("Unable to add watch %s",
> >                             error_get_pretty(local_err));
> > @@ -400,9 +465,17 @@ test_file_monitor_events(void)
> >              if (debug) {
> >                  g_printerr("Del watch %s %d\n", dir, op->watchid);
> >              }
> > +            if (op->filesrc && strchr(op->filesrc, '/')) {
> > +                watchdir = g_strdup_printf("%s/%s", dir, op->filesrc);
> > +                watchfile = strrchr(watchdir, '/');
> > +                *(char *)watchfile = '\0';
> > +            } else {
> > +                watchdir = g_strdup(dir);
> > +            }
> >              qemu_file_monitor_remove_watch(mon,
> > -                                           dir,
> > +                                           watchdir,
> >                                             op->watchid);
> > +            g_free(watchdir);
> >              break;
> >          case QFILE_MONITOR_TEST_OP_EVENT:
> >              if (debug) {
> > @@ -492,6 +565,28 @@ test_file_monitor_events(void)
> >              }
> >              break;
> >  
> > +        case QFILE_MONITOR_TEST_OP_MKDIR:
> > +            if (debug) {
> > +                g_printerr("Mkdir %s\n", pathsrc);
> > +            }
> > +            if (mkdir(pathsrc, 0700) < 0) {
> > +                g_printerr("Unable to mkdir %s: %s",
> > +                           pathsrc, strerror(errno));
> > +                goto cleanup;
> > +            }
> > +            break;
> > +
> > +        case QFILE_MONITOR_TEST_OP_RMDIR:
> > +            if (debug) {
> > +                g_printerr("Rmdir %s\n", pathsrc);
> > +            }
> > +            if (rmdir(pathsrc) < 0) {
> > +                g_printerr("Unable to rmdir %s: %s",
> > +                           pathsrc, strerror(errno));
> > +                goto cleanup;
> > +            }
> > +            break;
> > +
> >          default:
> >              g_assert_not_reached();
> >          }
> > @@ -532,13 +627,18 @@ test_file_monitor_events(void)
> >              const QFileMonitorTestOp *op = &(ops[i]);
> >              char *path = g_strdup_printf("%s/%s",
> >                                           dir, op->filesrc);
> > -            unlink(path);
> > -            g_free(path);
> > -            if (op->filedst) {
> > -                path = g_strdup_printf("%s/%s",
> > -                                       dir, op->filedst);
> > +            if (op->type == QFILE_MONITOR_TEST_OP_MKDIR) {
> > +                rmdir(path);
> > +                g_free(path);
> > +            } else {
> >                  unlink(path);
> >                  g_free(path);
> > +                if (op->filedst) {
> > +                    path = g_strdup_printf("%s/%s",
> > +                                           dir, op->filedst);
> > +                    unlink(path);
> > +                    g_free(path);
> > +                }
> >              }
> >          }
> >          if (rmdir(dir) < 0) {
> > diff --git a/util/filemonitor-inotify.c b/util/filemonitor-inotify.c
> > index 3a72be037f..3eb29f860b 100644
> > --- a/util/filemonitor-inotify.c
> > +++ b/util/filemonitor-inotify.c
> > @@ -29,7 +29,7 @@
> >  
> >  struct QFileMonitor {
> >      int fd;
> > -
> > +    int nextid; /* watch ID counter */
> >      QemuMutex lock; /* protects dirs & idmap */
> >      GHashTable *dirs; /* dirname => QFileMonitorDir */
> >      GHashTable *idmap; /* inotify ID => dirname */
> > @@ -47,7 +47,6 @@ typedef struct {
> >  typedef struct {
> >      char *path;
> >      int id; /* inotify ID */
> > -    int nextid; /* watch ID counter */
> >      GArray *watches; /* QFileMonitorWatch elements */
> >  } QFileMonitorDir;
> >  
> > @@ -277,7 +276,7 @@ qemu_file_monitor_add_watch(QFileMonitor *mon,
> >          }
> >      }
> >  
> > -    watch.id = dir->nextid++;
> > +    watch.id = mon->nextid++;
> >      watch.filename = g_strdup(filename);
> >      watch.cb = cb;
> >      watch.opaque = opaque;
> 
> Thanks, this fixes it! I had a related question about the way
> qemu_file_monitor_add_watch works.
> 
> Am I correct in understanding that if there is already a watch on a dir,
> we return back mon->nextid++ i.e the next free id. Why don't we return
> back the originally assigned watchid ?

inotify watches are a per-directory scope thing, but this API is aiming
to present the ability to have per-file scope watches as it is more
convenient to have those semantics for some users in QEMU. Thus many
QEMU level watches are mapped to the same low level inotify watch.

The watch ID is used to unregister a watch later, and while I could have
done ref counting against a common watch ID, I find it clearer to maintain
separate watch IDs for every thing the caller watches. If nothing else it
makes the debugging easier as you can see the relation between events
received and the original watch registration.

Regards,
Daniel
Bandan Das March 15, 2019, 5:24 p.m. UTC | #4
Daniel P. Berrangé <berrange@redhat.com> writes:
...
>> Thanks, this fixes it! I had a related question about the way
>> qemu_file_monitor_add_watch works.
>> 
>> Am I correct in understanding that if there is already a watch on a dir,
>> we return back mon->nextid++ i.e the next free id. Why don't we return
>> back the originally assigned watchid ?
>
> inotify watches are a per-directory scope thing, but this API is aiming
> to present the ability to have per-file scope watches as it is more
> convenient to have those semantics for some users in QEMU. Thus many
> QEMU level watches are mapped to the same low level inotify watch.
>
> The watch ID is used to unregister a watch later, and while I could have
> done ref counting against a common watch ID, I find it clearer to maintain
> separate watch IDs for every thing the caller watches. If nothing else it
> makes the debugging easier as you can see the relation between events
> received and the original watch registration.
>
Thanks, this makes sense. I think an advantage of having a refcounting
mechanism is that we won't run out of watchids because some caller
ended up adding a watch on the same dir repeatedly.

> Regards,
> Daniel
Daniel P. Berrangé March 15, 2019, 5:37 p.m. UTC | #5
On Fri, Mar 15, 2019 at 01:24:42PM -0400, Bandan Das wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> ...
> >> Thanks, this fixes it! I had a related question about the way
> >> qemu_file_monitor_add_watch works.
> >> 
> >> Am I correct in understanding that if there is already a watch on a dir,
> >> we return back mon->nextid++ i.e the next free id. Why don't we return
> >> back the originally assigned watchid ?
> >
> > inotify watches are a per-directory scope thing, but this API is aiming
> > to present the ability to have per-file scope watches as it is more
> > convenient to have those semantics for some users in QEMU. Thus many
> > QEMU level watches are mapped to the same low level inotify watch.
> >
> > The watch ID is used to unregister a watch later, and while I could have
> > done ref counting against a common watch ID, I find it clearer to maintain
> > separate watch IDs for every thing the caller watches. If nothing else it
> > makes the debugging easier as you can see the relation between events
> > received and the original watch registration.
> >
> Thanks, this makes sense. I think an advantage of having a refcounting
> mechanism is that we won't run out of watchids because some caller
> ended up adding a watch on the same dir repeatedly.

The watchid number is global to the QFileMonitor object, so if you
repeatedly create & delete watches on the same dir you're still going
to use up watch ids. You'll only save if you create & delete per-file
watches within the dir. Given that the guest OS can trigger create
/ delete of directories though, which in turn triggers create/delete
of watches, it looks like we should at the very least check for
overflow.

BTW, does USB-MTP have any limit on the number of objects it will
permit the guest to access ? something ought to limit the memory
usage of resources allocated per dir, including the watch state.

Regards,
Daniel
Bandan Das March 18, 2019, 2:58 p.m. UTC | #6
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Mar 15, 2019 at 01:24:42PM -0400, Bandan Das wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> ...
>> >> Thanks, this fixes it! I had a related question about the way
>> >> qemu_file_monitor_add_watch works.
>> >> 
>> >> Am I correct in understanding that if there is already a watch on a dir,
>> >> we return back mon->nextid++ i.e the next free id. Why don't we return
>> >> back the originally assigned watchid ?
>> >
>> > inotify watches are a per-directory scope thing, but this API is aiming
>> > to present the ability to have per-file scope watches as it is more
>> > convenient to have those semantics for some users in QEMU. Thus many
>> > QEMU level watches are mapped to the same low level inotify watch.
>> >
>> > The watch ID is used to unregister a watch later, and while I could have
>> > done ref counting against a common watch ID, I find it clearer to maintain
>> > separate watch IDs for every thing the caller watches. If nothing else it
>> > makes the debugging easier as you can see the relation between events
>> > received and the original watch registration.
>> >
>> Thanks, this makes sense. I think an advantage of having a refcounting
>> mechanism is that we won't run out of watchids because some caller
>> ended up adding a watch on the same dir repeatedly.
>
> The watchid number is global to the QFileMonitor object, so if you
> repeatedly create & delete watches on the same dir you're still going
> to use up watch ids. You'll only save if you create & delete per-file
> watches within the dir. Given that the guest OS can trigger create
> / delete of directories though, which in turn triggers create/delete
> of watches, it looks like we should at the very least check for
> overflow.
>
> BTW, does USB-MTP have any limit on the number of objects it will
> permit the guest to access ? something ought to limit the memory
> usage of resources allocated per dir, including the watch state.
>
No, it doesn't atm and is limited by memory. This does sound right
to prevent an overflow but I am not sure what would be a good limit
for a directory hierarchy with multiple files and folders.

Bandan

> Regards,
> Daniel

Patch
diff mbox series

diff --git a/tests/test-util-filemonitor.c b/tests/test-util-filemonitor.c
index ea3715a8f4..71a7cf5de0 100644
--- a/tests/test-util-filemonitor.c
+++ b/tests/test-util-filemonitor.c
@@ -35,6 +35,8 @@  enum {
     QFILE_MONITOR_TEST_OP_RENAME,
     QFILE_MONITOR_TEST_OP_TOUCH,
     QFILE_MONITOR_TEST_OP_UNLINK,
+    QFILE_MONITOR_TEST_OP_MKDIR,
+    QFILE_MONITOR_TEST_OP_RMDIR,
 };
 
 typedef struct {
@@ -298,6 +300,54 @@  test_file_monitor_events(void)
           .eventid = QFILE_MONITOR_EVENT_DELETED },
 
 
+        { .type = QFILE_MONITOR_TEST_OP_MKDIR,
+          .filesrc = "fish", },
+        { .type = QFILE_MONITOR_TEST_OP_EVENT,
+          .filesrc = "fish", .watchid = 0,
+          .eventid = QFILE_MONITOR_EVENT_CREATED },
+
+
+        { .type = QFILE_MONITOR_TEST_OP_ADD_WATCH,
+          .filesrc = "fish/", .watchid = 4 },
+        { .type = QFILE_MONITOR_TEST_OP_ADD_WATCH,
+          .filesrc = "fish/one.txt", .watchid = 5 },
+        { .type = QFILE_MONITOR_TEST_OP_CREATE,
+          .filesrc = "fish/one.txt", },
+        { .type = QFILE_MONITOR_TEST_OP_EVENT,
+          .filesrc = "one.txt", .watchid = 4,
+          .eventid = QFILE_MONITOR_EVENT_CREATED },
+        { .type = QFILE_MONITOR_TEST_OP_EVENT,
+          .filesrc = "one.txt", .watchid = 5,
+          .eventid = QFILE_MONITOR_EVENT_CREATED },
+
+
+        { .type = QFILE_MONITOR_TEST_OP_DEL_WATCH,
+          .filesrc = "fish/one.txt", .watchid = 5 },
+        { .type = QFILE_MONITOR_TEST_OP_RENAME,
+          .filesrc = "fish/one.txt", .filedst = "two.txt", },
+        { .type = QFILE_MONITOR_TEST_OP_EVENT,
+          .filesrc = "one.txt", .watchid = 4,
+          .eventid = QFILE_MONITOR_EVENT_DELETED },
+        { .type = QFILE_MONITOR_TEST_OP_EVENT,
+          .filesrc = "two.txt", .watchid = 0,
+          .eventid = QFILE_MONITOR_EVENT_CREATED },
+        { .type = QFILE_MONITOR_TEST_OP_EVENT,
+          .filesrc = "two.txt", .watchid = 2,
+          .eventid = QFILE_MONITOR_EVENT_CREATED },
+
+
+        { .type = QFILE_MONITOR_TEST_OP_RMDIR,
+          .filesrc = "fish", },
+        { .type = QFILE_MONITOR_TEST_OP_EVENT,
+          .filesrc = "", .watchid = 4,
+          .eventid = QFILE_MONITOR_EVENT_IGNORED },
+        { .type = QFILE_MONITOR_TEST_OP_EVENT,
+          .filesrc = "fish", .watchid = 0,
+          .eventid = QFILE_MONITOR_EVENT_DELETED },
+        { .type = QFILE_MONITOR_TEST_OP_DEL_WATCH,
+          .filesrc = "fish", .watchid = 4 },
+
+
         { .type = QFILE_MONITOR_TEST_OP_UNLINK,
           .filesrc = "two.txt", },
         { .type = QFILE_MONITOR_TEST_OP_EVENT,
@@ -366,6 +416,8 @@  test_file_monitor_events(void)
         int fd;
         int watchid;
         struct utimbuf ubuf;
+        char *watchdir;
+        const char *watchfile;
 
         pathsrc = g_strdup_printf("%s/%s", dir, op->filesrc);
         if (op->filedst) {
@@ -378,13 +430,26 @@  test_file_monitor_events(void)
                 g_printerr("Add watch %s %s %d\n",
                            dir, op->filesrc, op->watchid);
             }
+            if (op->filesrc && strchr(op->filesrc, '/')) {
+                watchdir = g_strdup_printf("%s/%s", dir, op->filesrc);
+                watchfile = strrchr(watchdir, '/');
+                *(char *)watchfile = '\0';
+                watchfile++;
+                if (*watchfile == '\0') {
+                    watchfile = NULL;
+                }
+            } else {
+                watchdir = g_strdup(dir);
+                watchfile = op->filesrc;
+            }
             watchid =
                 qemu_file_monitor_add_watch(mon,
-                                            dir,
-                                            op->filesrc,
+                                            watchdir,
+                                            watchfile,
                                             qemu_file_monitor_test_handler,
                                             &data,
                                             &local_err);
+            g_free(watchdir);
             if (watchid < 0) {
                 g_printerr("Unable to add watch %s",
                            error_get_pretty(local_err));
@@ -400,9 +465,17 @@  test_file_monitor_events(void)
             if (debug) {
                 g_printerr("Del watch %s %d\n", dir, op->watchid);
             }
+            if (op->filesrc && strchr(op->filesrc, '/')) {
+                watchdir = g_strdup_printf("%s/%s", dir, op->filesrc);
+                watchfile = strrchr(watchdir, '/');
+                *(char *)watchfile = '\0';
+            } else {
+                watchdir = g_strdup(dir);
+            }
             qemu_file_monitor_remove_watch(mon,
-                                           dir,
+                                           watchdir,
                                            op->watchid);
+            g_free(watchdir);
             break;
         case QFILE_MONITOR_TEST_OP_EVENT:
             if (debug) {
@@ -492,6 +565,28 @@  test_file_monitor_events(void)
             }
             break;
 
+        case QFILE_MONITOR_TEST_OP_MKDIR:
+            if (debug) {
+                g_printerr("Mkdir %s\n", pathsrc);
+            }
+            if (mkdir(pathsrc, 0700) < 0) {
+                g_printerr("Unable to mkdir %s: %s",
+                           pathsrc, strerror(errno));
+                goto cleanup;
+            }
+            break;
+
+        case QFILE_MONITOR_TEST_OP_RMDIR:
+            if (debug) {
+                g_printerr("Rmdir %s\n", pathsrc);
+            }
+            if (rmdir(pathsrc) < 0) {
+                g_printerr("Unable to rmdir %s: %s",
+                           pathsrc, strerror(errno));
+                goto cleanup;
+            }
+            break;
+
         default:
             g_assert_not_reached();
         }
@@ -532,13 +627,18 @@  test_file_monitor_events(void)
             const QFileMonitorTestOp *op = &(ops[i]);
             char *path = g_strdup_printf("%s/%s",
                                          dir, op->filesrc);
-            unlink(path);
-            g_free(path);
-            if (op->filedst) {
-                path = g_strdup_printf("%s/%s",
-                                       dir, op->filedst);
+            if (op->type == QFILE_MONITOR_TEST_OP_MKDIR) {
+                rmdir(path);
+                g_free(path);
+            } else {
                 unlink(path);
                 g_free(path);
+                if (op->filedst) {
+                    path = g_strdup_printf("%s/%s",
+                                           dir, op->filedst);
+                    unlink(path);
+                    g_free(path);
+                }
             }
         }
         if (rmdir(dir) < 0) {
diff --git a/util/filemonitor-inotify.c b/util/filemonitor-inotify.c
index 3a72be037f..3eb29f860b 100644
--- a/util/filemonitor-inotify.c
+++ b/util/filemonitor-inotify.c
@@ -29,7 +29,7 @@ 
 
 struct QFileMonitor {
     int fd;
-
+    int nextid; /* watch ID counter */
     QemuMutex lock; /* protects dirs & idmap */
     GHashTable *dirs; /* dirname => QFileMonitorDir */
     GHashTable *idmap; /* inotify ID => dirname */
@@ -47,7 +47,6 @@  typedef struct {
 typedef struct {
     char *path;
     int id; /* inotify ID */
-    int nextid; /* watch ID counter */
     GArray *watches; /* QFileMonitorWatch elements */
 } QFileMonitorDir;
 
@@ -277,7 +276,7 @@  qemu_file_monitor_add_watch(QFileMonitor *mon,
         }
     }
 
-    watch.id = dir->nextid++;
+    watch.id = mon->nextid++;
     watch.filename = g_strdup(filename);
     watch.cb = cb;
     watch.opaque = opaque;