On Thu, 2005-09-22 at 16:35 +1000, Rusty Russell wrote:
> Hi Christian!
>
> I was looking through b0893b876c8c4c4eb507d48fc1c4af4268ddecde, and it
> took me a while to figure out what the code was doing. I think there
> are two issues here:
>
> 1) Directories implicitly created by mkdir/write don't fire watches (eg.
> mkdir /a/sub/dir only fires a watch event for /a/sub/dir, even if it
> also created /a/sub), and
> 2) Rm only fires a single event for a watch, as an optimization.
>
> The first one is a bug, I think, and I'm testing a patch as I type this.
Indeed, here is the patch. Applies on top of those 3 tdb-conversion
patches...
Rusty.
# HG changeset patch
# User Rusty Russell <rusty@xxxxxxxxxxxxxxx>
# Node ID 5ea1c70f0adb9625a5640b61c9068923a95cb8d7
# Parent 6ec88332e05dd2433ca568de619b3e809f66ca41
Fire watches on implicitly-created directories.
This patch separates watch event creation and firing, which improves robustness
a little should we run out of memory, as well as allowing us to build up a
series of different events (such as for each directory created in a write/mkdir
which creates more than one), and fire them on successful completion.
Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
diff -r 6ec88332e05d -r 5ea1c70f0adb tools/xenstore/testsuite/07watch.test
--- a/tools/xenstore/testsuite/07watch.test Wed Sep 21 05:44:48 2005
+++ b/tools/xenstore/testsuite/07watch.test Thu Sep 22 06:42:54 2005
@@ -184,6 +184,7 @@
expect 1:/test/subnode:token
1 waitwatch
1 ackwatch token
+1 close
# Watch should not double-send after we ack, even if we did something in
between.
1 watch /test2 token
@@ -195,3 +196,32 @@
1 ackwatch token
expect 1: waitwatch failed: Connection timed out
1 waitwatch
+1 close
+
+# Watch fires on implicitly-created directories (mkdir)
+1 watch /test2 token
+2 mkdir /test2/sub/dir/dir2
+expect 1:/test2/sub:token
+1 waitwatch
+1 ackwatch token
+expect 1:/test2/sub/dir:token
+1 waitwatch
+1 ackwatch token
+expect 1:/test2/sub/dir/dir2:token
+1 waitwatch
+1 ackwatch token
+1 close
+
+# Watch fires on implicitly-created directories (write)
+1 watch /test2 token
+2 write /test2/sub2/dir/entry contents
+expect 1:/test2/sub2:token
+1 waitwatch
+1 ackwatch token
+expect 1:/test2/sub2/dir:token
+1 waitwatch
+1 ackwatch token
+expect 1:/test2/sub2/dir/entry:token
+1 waitwatch
+1 ackwatch token
+1 close
diff -r 6ec88332e05d -r 5ea1c70f0adb tools/xenstore/xenstored_core.c
--- a/tools/xenstore/xenstored_core.c Wed Sep 21 05:44:48 2005
+++ b/tools/xenstore/xenstored_core.c Thu Sep 22 06:42:54 2005
@@ -747,7 +747,8 @@
return strrchr(name, '/') + 1;
}
-static struct node *construct_node(struct connection *conn, const char *name)
+static struct node *construct_node(struct connection *conn, const char *name,
+ struct events **ev)
{
const char *base;
unsigned int baselen;
@@ -757,7 +758,7 @@
/* If parent doesn't exist, create it. */
parent = read_node(conn, parentname);
if (!parent)
- parent = construct_node(conn, parentname);
+ parent = construct_node(conn, parentname, ev);
if (!parent)
return NULL;
@@ -774,6 +775,7 @@
node = talloc(name, struct node);
node->tdb = tdb_context(conn);
node->name = talloc_strdup(node, name);
+ *ev = add_events(conn, name, false, *ev);
/* Inherit permissions, except domains own what they create */
node->num_perms = parent->num_perms;
@@ -808,11 +810,12 @@
* This helps fsck if we die during this. */
static struct node *create_node(struct connection *conn,
const char *name,
- void *data, unsigned int datalen)
+ void *data, unsigned int datalen,
+ struct events **ev)
{
struct node *node, *i;
- node = construct_node(conn, name);
+ node = construct_node(conn, name, ev);
if (!node)
return NULL;
@@ -838,6 +841,7 @@
{
unsigned int offset, datalen;
struct node *node;
+ struct events *ev = NULL;
char *vec[1] = { NULL }; /* gcc4 + -W + -Werror fucks code. */
char *name;
@@ -858,7 +862,8 @@
send_error(conn, errno);
return;
}
- node = create_node(conn, name, in->buffer + offset, datalen);
+ node = create_node(conn, name, in->buffer + offset, datalen,
+ &ev);
if (!node) {
send_error(conn, errno);
return;
@@ -866,6 +871,7 @@
} else {
node->data = in->buffer + offset;
node->datalen = datalen;
+ ev = add_events(conn, name, false, NULL);
if (!write_node(conn, node)){
send_error(conn, errno);
return;
@@ -873,13 +879,14 @@
}
add_change_node(conn->transaction, name, false);
- fire_watches(conn, name, false);
+ fire_events(ev);
send_ack(conn, XS_WRITE);
}
static void do_mkdir(struct connection *conn, const char *name)
{
struct node *node;
+ struct events *ev = NULL;
name = canonicalize(conn, name);
node = get_node(conn, name, XS_PERM_WRITE);
@@ -891,13 +898,13 @@
send_error(conn, errno);
return;
}
- node = create_node(conn, name, NULL, 0);
+ node = create_node(conn, name, NULL, 0, &ev);
if (!node) {
send_error(conn, errno);
return;
}
add_change_node(conn->transaction, name, false);
- fire_watches(conn, name, false);
+ fire_events(ev);
}
send_ack(conn, XS_MKDIR);
}
@@ -948,6 +955,7 @@
static void do_rm(struct connection *conn, const char *name)
{
struct node *node, *parent;
+ struct events *ev = NULL;
name = canonicalize(conn, name);
node = get_node(conn, name, XS_PERM_WRITE);
@@ -978,6 +986,7 @@
return;
}
+ ev = add_events(conn, name, true, NULL);
if (!delete_child(conn, parent, basename(name))) {
send_error(conn, EINVAL);
return;
@@ -985,7 +994,7 @@
delete_node(conn, node);
add_change_node(conn->transaction, name, true);
- fire_watches(conn, name, true);
+ fire_events(ev);
send_ack(conn, XS_RM);
}
@@ -1014,6 +1023,7 @@
unsigned int num;
char *name, *permstr;
struct node *node;
+ struct events *ev;
num = xs_count_strings(in->buffer, in->used);
if (num < 2) {
@@ -1033,6 +1043,7 @@
return;
}
+ ev = add_events(conn, name, false, NULL);
node->perms = talloc_array(node, struct xs_permissions, num);
node->num_perms = num;
if (!xs_strings_to_perms(node->perms, num, permstr)) {
@@ -1045,7 +1056,7 @@
}
add_change_node(conn->transaction, name, false);
- fire_watches(conn, name, false);
+ fire_events(ev);
send_ack(conn, XS_SET_PERMS);
}
diff -r 6ec88332e05d -r 5ea1c70f0adb tools/xenstore/xenstored_domain.c
--- a/tools/xenstore/xenstored_domain.c Wed Sep 21 05:44:48 2005
+++ b/tools/xenstore/xenstored_domain.c Thu Sep 22 06:42:54 2005
@@ -235,7 +235,7 @@
}
if (released)
- fire_watches(NULL, "@releaseDomain", false);
+ special_event("@releaseDomain");
}
/* We scan all domains rather than use the information given here. */
@@ -330,8 +330,7 @@
/* Now domain belongs to its connection. */
talloc_steal(domain->conn, domain);
- fire_watches(conn, "@introduceDomain", false);
-
+ special_event("@introduceDomain");
send_ack(conn, XS_INTRODUCE);
}
@@ -378,10 +377,9 @@
send_error(conn, EINVAL);
return;
}
-
talloc_free(domain->conn);
- fire_watches(conn, "@releaseDomain", false);
+ special_event("@releaseDomain");
send_ack(conn, XS_RELEASE);
}
diff -r 6ec88332e05d -r 5ea1c70f0adb tools/xenstore/xenstored_transaction.c
--- a/tools/xenstore/xenstored_transaction.c Wed Sep 21 05:44:48 2005
+++ b/tools/xenstore/xenstored_transaction.c Thu Sep 22 06:42:54 2005
@@ -161,11 +161,18 @@
talloc_steal(arg, trans);
if (streq(arg, "T")) {
+ struct events *ev = NULL;
+
/* FIXME: Merge, rather failing on any change. */
if (trans->generation != generation) {
send_error(conn, EAGAIN);
return;
}
+
+ /* Allocate watch events to send *before* commit. */
+ list_for_each_entry(i, &trans->changes, list)
+ ev = add_events(conn, i->node, i->recurse, ev);
+
if (!replace_tdb(trans->tdb_name, trans->tdb)) {
send_error(conn, errno);
return;
@@ -173,9 +180,7 @@
/* Don't close this: we won! */
trans->tdb = NULL;
- /* Fire off the watches for everything that changed. */
- list_for_each_entry(i, &trans->changes, list)
- fire_watches(conn, i->node, i->recurse);
+ fire_events(ev);
generation++;
}
send_ack(conn, XS_TRANSACTION_END);
diff -r 6ec88332e05d -r 5ea1c70f0adb tools/xenstore/xenstored_watch.c
--- a/tools/xenstore/xenstored_watch.c Wed Sep 21 05:44:48 2005
+++ b/tools/xenstore/xenstored_watch.c Thu Sep 22 06:42:54 2005
@@ -38,6 +38,8 @@
/* The events on this watch. */
struct list_head list;
+ struct watch *watch;
+
/* Data to send (node\0token\0). */
unsigned int len;
char *data;
@@ -54,8 +56,17 @@
/* Is this relative to connnection's implicit path? */
const char *relative_path;
+ /* Connection which placed this watch. */
+ struct connection *conn;
+
char *token;
char *node;
+};
+
+struct events
+{
+ unsigned int num;
+ struct watch_event **events;
};
/* Look through our watches: if any of them have an event, queue it. */
@@ -91,11 +102,13 @@
{
struct watch_event *event = _event;
+ list_del(&event->list);
trace_destroy(event, "watch_event");
return 0;
}
-static void add_event(struct connection *conn,
+static void add_event(struct events *ev,
+ struct connection *conn,
struct watch *watch,
const char *name)
{
@@ -116,7 +129,10 @@
name++;
}
- event = talloc(watch, struct watch_event);
+ ev->events = talloc_realloc(ev, ev->events, struct watch_event *,
+ ev->num + 1);
+ ev->events[ev->num++] = event = talloc(ev, struct watch_event);
+ event->watch = watch;
event->len = strlen(name) + 1 + strlen(watch->token) + 1;
event->data = talloc_array(event, char, event->len);
strcpy(event->data, name);
@@ -126,30 +142,38 @@
trace_create(event, "watch_event");
}
-/* FIXME: we fail to fire on out of memory. Should drop connections. */
-void fire_watches(struct connection *conn, const char *name, bool recurse)
+static struct events *new_events(const void *ctx)
+{
+ struct events *ev = talloc(ctx, struct events);
+ ev->num = 0;
+ ev->events = NULL;
+ return ev;
+}
+
+struct events *add_events(struct connection *conn,
+ const char *name,
+ bool recurse,
+ struct events *old)
{
struct connection *i;
struct watch *watch;
/* During transactions, don't fire watches. */
- if (conn && conn->transaction)
- return;
-
- /* Create an event for each watch. */
+ if (conn->transaction)
+ return NULL;
+
+ if (!old)
+ old = new_events(name);
+
list_for_each_entry(i, &connections, list) {
list_for_each_entry(watch, &i->watches, list) {
if (is_child(name, watch->node))
- add_event(i, watch, name);
+ add_event(old, i, watch, name);
else if (recurse && is_child(watch->node, name))
- add_event(i, watch, watch->node);
- else
- continue;
- /* If connection not doing anything, queue this. */
- if (i->state == OK)
- queue_next_event(i);
- }
- }
+ add_event(old, i, watch, watch->node);
+ }
+ }
+ return old;
}
static int destroy_watch(void *_watch)
@@ -158,11 +182,40 @@
return 0;
}
+/* Create a single event for this watch. */
+static struct events *single_event(void *ctx,
+ struct connection *conn,
+ struct watch *watch)
+{
+ struct events *ev = new_events(ctx);
+
+ add_event(ev, conn, watch, watch->node);
+ return ev;
+}
+
+/* No convenient context to hang this off, so free immediately. */
+void special_event(const char *eventname)
+{
+ struct connection *i;
+ struct watch *watch;
+ struct events *ev = new_events(talloc_autofree_context());
+
+ list_for_each_entry(i, &connections, list) {
+ list_for_each_entry(watch, &i->watches, list) {
+ if (streq(eventname, watch->node))
+ add_event(ev, i, watch, watch->node);
+ }
+ }
+ fire_events(ev);
+ talloc_free(ev);
+}
+
void do_watch(struct connection *conn, struct buffered_data *in)
{
struct watch *watch;
char *vec[2];
bool relative;
+ struct events *ev;
if (get_strings(in, vec, ARRAY_SIZE(vec)) != ARRAY_SIZE(vec)) {
send_error(conn, EINVAL);
@@ -184,6 +237,7 @@
watch = talloc(conn, struct watch);
watch->node = talloc_strdup(watch, vec[0]);
watch->token = talloc_strdup(watch, vec[1]);
+ watch->conn = conn;
if (relative)
watch->relative_path = get_implicit_path(conn);
else
@@ -194,16 +248,30 @@
list_add_tail(&watch->list, &conn->watches);
trace_create(watch, "watch");
talloc_set_destructor(watch, destroy_watch);
+ /* We fire once up front: simplifies clients and restart. */
+ ev = single_event(in, conn, watch);
send_ack(conn, XS_WATCH);
-
- /* We fire once up front: simplifies clients and restart. */
- add_event(conn, watch, watch->node);
+ fire_events(ev);
+}
+
+void fire_events(struct events *ev)
+{
+ unsigned int i;
+
+ if (!ev)
+ return;
+
+ /* Reparent each event to the corresponding watch. */
+ for (i = 0; i < ev->num; i++) {
+ talloc_steal(ev->events[i]->watch, ev->events[i]);
+ /* If connection not doing anything, queue this. */
+ if (ev->events[i]->watch->conn->state == OK)
+ queue_next_event(ev->events[i]->watch->conn);
+ }
}
void do_watch_ack(struct connection *conn, const char *token)
{
- struct watch_event *event;
-
if (!token) {
send_error(conn, EINVAL);
return;
@@ -222,10 +290,8 @@
}
/* Remove event: after ack sent, core will call queue_next_event */
- event = list_top(&conn->waiting_for_ack->events, struct watch_event,
- list);
- list_del(&event->list);
- talloc_free(event);
+ talloc_free(list_top(&conn->waiting_for_ack->events,
+ struct watch_event, list));
conn->waiting_for_ack = NULL;
send_ack(conn, XS_WATCH_ACK);
diff -r 6ec88332e05d -r 5ea1c70f0adb tools/xenstore/xenstored_watch.h
--- a/tools/xenstore/xenstored_watch.h Wed Sep 21 05:44:48 2005
+++ b/tools/xenstore/xenstored_watch.h Thu Sep 22 06:42:54 2005
@@ -32,9 +32,20 @@
/* Look through our watches: if any of them have an event, queue it. */
void queue_next_event(struct connection *conn);
-/* Fire all watches: recurse means all the children are affected (ie. rm).
- */
-void fire_watches(struct connection *conn, const char *name, bool recurse);
+struct events;
+
+/* Create watch events: recurse means all the children are effected (ie. rm).
+ * Add to old events, if that is non-NULL. */
+struct events *add_events(struct connection *conn,
+ const char *name,
+ bool recurse,
+ struct events *old);
+
+/* Actually fire those watch events */
+void fire_events(struct events *events);
+
+/* Fire a special event (introduce or release). */
+void special_event(const char *eventname);
void dump_watches(struct connection *conn);
--
A bad analogy is like a leaky screwdriver -- Richard Braakman
_______________________________________________
Xen-tools mailing list
Xen-tools@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-tools
|
Home