| Message ID | 1478851210-6251-3-git-send-email-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 11, 2016 at 09:00:00AM +0100, Juergen Gross wrote: > Instead of calling add_change_node() at places where write_node() is > called, do that inside write_node(). > > Signed-off-by: Juergen Gross <jgross@suse.com> There seems to be a subtle change in behaviour -- previously in create_node, there is no add_chnage_node called. Now it has. > --- > tools/xenstore/xenstored_core.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c > index de1a9b4..1354387 100644 > --- a/tools/xenstore/xenstored_core.c > +++ b/tools/xenstore/xenstored_core.c > @@ -456,7 +456,7 @@ static struct node *read_node(struct connection *conn, const void *ctx, > return node; > } > > -static bool write_node(struct connection *conn, const struct node *node) > +static bool write_node(struct connection *conn, struct node *node) > { > /* > * conn will be null when this is called from manual_node. > @@ -476,6 +476,8 @@ static bool write_node(struct connection *conn, const struct node *node) > if (domain_is_unprivileged(conn) && data.dsize >= quota_max_entry_size) > goto error; > > + add_change_node(conn, node, false); > + Another subtle change of behaviour -- there is another goto error after this, which means the change is not made as far as the caller is concerned if that path is taken. Not saying that all these changes are wrong, but they are worth pointing out and probably we should put the reasoning into commit message. > data.dptr = talloc_size(node, data.dsize); > ((uint32_t *)data.dptr)[0] = node->num_perms; > ((uint32_t *)data.dptr)[1] = node->datalen; > @@ -976,7 +978,6 @@ static void do_write(struct connection *conn, struct buffered_data *in) > } > } > > - add_change_node(conn, node, false); > fire_watches(conn, in, name, false); > send_ack(conn, XS_WRITE); > } > @@ -1007,7 +1008,6 @@ static void do_mkdir(struct connection *conn, struct buffered_data *in) > send_error(conn, errno); > return; > } > - add_change_node(conn, node, false); > fire_watches(conn, in, name, false); > } > send_ack(conn, XS_MKDIR); > @@ -1209,7 +1209,6 @@ static void do_set_perms(struct connection *conn, struct buffered_data *in) > return; > } > > - add_change_node(conn, node, false); > fire_watches(conn, in, name, false); > send_ack(conn, XS_SET_PERMS); > } > -- > 2.6.6 >
On 12/11/16 16:10, Wei Liu wrote: > On Fri, Nov 11, 2016 at 09:00:00AM +0100, Juergen Gross wrote: >> Instead of calling add_change_node() at places where write_node() is >> called, do that inside write_node(). >> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > > There seems to be a subtle change in behaviour -- previously in > create_node, there is no add_chnage_node called. Now it has. No change. add_change_node() has been called after calling create_node() > >> --- >> tools/xenstore/xenstored_core.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c >> index de1a9b4..1354387 100644 >> --- a/tools/xenstore/xenstored_core.c >> +++ b/tools/xenstore/xenstored_core.c >> @@ -456,7 +456,7 @@ static struct node *read_node(struct connection *conn, const void *ctx, >> return node; >> } >> >> -static bool write_node(struct connection *conn, const struct node *node) >> +static bool write_node(struct connection *conn, struct node *node) >> { >> /* >> * conn will be null when this is called from manual_node. >> @@ -476,6 +476,8 @@ static bool write_node(struct connection *conn, const struct node *node) >> if (domain_is_unprivileged(conn) && data.dsize >= quota_max_entry_size) >> goto error; >> >> + add_change_node(conn, node, false); >> + > > Another subtle change of behaviour -- there is another goto error after > this, which means the change is not made as far as the caller is > concerned if that path is taken. That's a case which should not be of any concern: it means that the xenstore data base is corrupt. A stale watch event is the least problem then. > Not saying that all these changes are wrong, but they are worth pointing > out and probably we should put the reasoning into commit message. I'll add some words for this case. Juergen > >> data.dptr = talloc_size(node, data.dsize); >> ((uint32_t *)data.dptr)[0] = node->num_perms; >> ((uint32_t *)data.dptr)[1] = node->datalen; >> @@ -976,7 +978,6 @@ static void do_write(struct connection *conn, struct buffered_data *in) >> } >> } >> >> - add_change_node(conn, node, false); >> fire_watches(conn, in, name, false); >> send_ack(conn, XS_WRITE); >> } >> @@ -1007,7 +1008,6 @@ static void do_mkdir(struct connection *conn, struct buffered_data *in) >> send_error(conn, errno); >> return; >> } >> - add_change_node(conn, node, false); >> fire_watches(conn, in, name, false); >> } >> send_ack(conn, XS_MKDIR); >> @@ -1209,7 +1209,6 @@ static void do_set_perms(struct connection *conn, struct buffered_data *in) >> return; >> } >> >> - add_change_node(conn, node, false); >> fire_watches(conn, in, name, false); >> send_ack(conn, XS_SET_PERMS); >> } >> -- >> 2.6.6 >> >
On Sun, Nov 13, 2016 at 11:50:27AM +0100, Juergen Gross wrote: > On 12/11/16 16:10, Wei Liu wrote: > > On Fri, Nov 11, 2016 at 09:00:00AM +0100, Juergen Gross wrote: > >> Instead of calling add_change_node() at places where write_node() is > >> called, do that inside write_node(). > >> > >> Signed-off-by: Juergen Gross <jgross@suse.com> > > > > > > There seems to be a subtle change in behaviour -- previously in > > create_node, there is no add_chnage_node called. Now it has. > > No change. add_change_node() has been called after calling create_node() > I don't think that's true for all cases. For example, setup_structure calls create_node but doesn't call add_change_node after that. But I have convinced myself that it would be fine to call add_change_node in write_node because that seems rather logical -- writing a node is certainly changing it. Wei.
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index de1a9b4..1354387 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -456,7 +456,7 @@ static struct node *read_node(struct connection *conn, const void *ctx, return node; } -static bool write_node(struct connection *conn, const struct node *node) +static bool write_node(struct connection *conn, struct node *node) { /* * conn will be null when this is called from manual_node. @@ -476,6 +476,8 @@ static bool write_node(struct connection *conn, const struct node *node) if (domain_is_unprivileged(conn) && data.dsize >= quota_max_entry_size) goto error; + add_change_node(conn, node, false); + data.dptr = talloc_size(node, data.dsize); ((uint32_t *)data.dptr)[0] = node->num_perms; ((uint32_t *)data.dptr)[1] = node->datalen; @@ -976,7 +978,6 @@ static void do_write(struct connection *conn, struct buffered_data *in) } } - add_change_node(conn, node, false); fire_watches(conn, in, name, false); send_ack(conn, XS_WRITE); } @@ -1007,7 +1008,6 @@ static void do_mkdir(struct connection *conn, struct buffered_data *in) send_error(conn, errno); return; } - add_change_node(conn, node, false); fire_watches(conn, in, name, false); } send_ack(conn, XS_MKDIR); @@ -1209,7 +1209,6 @@ static void do_set_perms(struct connection *conn, struct buffered_data *in) return; } - add_change_node(conn, node, false); fire_watches(conn, in, name, false); send_ack(conn, XS_SET_PERMS); }
Instead of calling add_change_node() at places where write_node() is called, do that inside write_node(). Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenstore/xenstored_core.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)