Daniel P. Berrangé <berra...@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é <berra...@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 <b...@redhat.com>