Re: [Qemu-devel] [PATCH 2/2] filemon: ensure watch IDs are unique to QFileMonitor scope

[Bandan Das]

Daniel P. Berrangé <address@hidden> 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é <address@hidden>
> ---
>  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 <address@hidden>