Re: [PULL 05/27] hw/xen: Watches on XenStore transactions
On Tue, 2023-06-20 at 13:19 +0100, Peter Maydell wrote: > On Fri, 2 Jun 2023 at 18:06, Peter Maydell > wrote: > > > > On Tue, 2 May 2023 at 18:08, Peter Maydell > > wrote: > > > > > > On Tue, 7 Mar 2023 at 18:27, David Woodhouse > > > wrote: > > > > > > > > From: David Woodhouse > > > > Hi; Coverity's "is there missing error handling?" > > > heuristic fired for a change in this code (CID 1508359): > > > > > > > static int transaction_commit(XenstoreImplState *s, > > > > XsTransaction *tx) > > > > { > > > > + struct walk_op op; > > > > + XsNode **n; > > > > + > > > > if (s->root_tx != tx->base_tx) { > > > > return EAGAIN; > > > > } > > > > @@ -720,10 +861,18 @@ static int > > > > transaction_commit(XenstoreImplState *s, XsTransaction *tx) > > > > s->root_tx = tx->tx_id; > > > > s->nr_nodes = tx->nr_nodes; > > > > > > > > + init_walk_op(s, , XBT_NULL, tx->dom_id, "/", ); > > > > > > This is the only call to init_walk_op() which ignores its > > > return value. Intentional, or missing error handling? > > > > Hi -- I was going through the unclassified Coverity issues > > again today, and this one's still on the list. Is this a > > bug, or intentional? > > Ping^3 -- is this a false positive, or something to be fixed? > It would be nice to be able to classify the coverity issue > appropriately. Oops, sorry for the delay. It is arguably a false positive. There are two cases where init_walk_op() can fail: • It's given a transaction ID which doesn't exist. But in this case it's actually given XBT_NULL because the transaction is *already* committed and all we're doing is setting up a tree walk to fire watches on the newly-committed changed nodes. or, • The given path is invalid. Which it isn't here because we pass a hard-coded "/". I was about to stick in the standard if(ret){return ret;} but the semantics of that would be a bit bizarre. As noted, by this point the transaction *was* committed already. So all that gets aborted is the *watches* that were supposed to fire on changed nodes. Returning an error in that case would be a bit weird. So I'll go for a g_assert(!ret) with a comment about why. Patch follows. I shall also have another go at frowning at the soft-reset locking vs. the timer and other code, and seeing if I win this time... smime.p7s Description: S/MIME cryptographic signature
Re: [PULL 05/27] hw/xen: Watches on XenStore transactions
On Fri, 2 Jun 2023 at 18:06, Peter Maydell wrote: > > On Tue, 2 May 2023 at 18:08, Peter Maydell wrote: > > > > On Tue, 7 Mar 2023 at 18:27, David Woodhouse wrote: > > > > > > From: David Woodhouse > > Hi; Coverity's "is there missing error handling?" > > heuristic fired for a change in this code (CID 1508359): > > > > > static int transaction_commit(XenstoreImplState *s, XsTransaction *tx) > > > { > > > +struct walk_op op; > > > +XsNode **n; > > > + > > > if (s->root_tx != tx->base_tx) { > > > return EAGAIN; > > > } > > > @@ -720,10 +861,18 @@ static int transaction_commit(XenstoreImplState *s, > > > XsTransaction *tx) > > > s->root_tx = tx->tx_id; > > > s->nr_nodes = tx->nr_nodes; > > > > > > +init_walk_op(s, , XBT_NULL, tx->dom_id, "/", ); > > > > This is the only call to init_walk_op() which ignores its > > return value. Intentional, or missing error handling? > > Hi -- I was going through the unclassified Coverity issues > again today, and this one's still on the list. Is this a > bug, or intentional? Ping^3 -- is this a false positive, or something to be fixed? It would be nice to be able to classify the coverity issue appropriately. thanks -- PMM
Re: [PULL 05/27] hw/xen: Watches on XenStore transactions
On Tue, 2 May 2023 at 18:08, Peter Maydell wrote: > > On Tue, 7 Mar 2023 at 18:27, David Woodhouse wrote: > > > > From: David Woodhouse > > > > Firing watches on the nodes that still exist is relatively easy; just > > walk the tree and look at the nodes with refcount of one. > > > > Firing watches on *deleted* nodes is more fun. We add 'modified_in_tx' > > and 'deleted_in_tx' flags to each node. Nodes with those flags cannot > > be shared, as they will always be unique to the transaction in which > > they were created. > > > > When xs_node_walk would need to *create* a node as scaffolding and it > > encounters a deleted_in_tx node, it can resurrect it simply by clearing > > its deleted_in_tx flag. If that node originally had any *data*, they're > > gone, and the modified_in_tx flag will have been set when it was first > > deleted. > > > > We then attempt to send appropriate watches when the transaction is > > committed, properly delete the deleted_in_tx nodes, and remove the > > modified_in_tx flag from the others. > > > > Signed-off-by: David Woodhouse > > Reviewed-by: Paul Durrant > > Hi; Coverity's "is there missing error handling?" > heuristic fired for a change in this code (CID 1508359): > > > static int transaction_commit(XenstoreImplState *s, XsTransaction *tx) > > { > > +struct walk_op op; > > +XsNode **n; > > + > > if (s->root_tx != tx->base_tx) { > > return EAGAIN; > > } > > @@ -720,10 +861,18 @@ static int transaction_commit(XenstoreImplState *s, > > XsTransaction *tx) > > s->root_tx = tx->tx_id; > > s->nr_nodes = tx->nr_nodes; > > > > +init_walk_op(s, , XBT_NULL, tx->dom_id, "/", ); > > This is the only call to init_walk_op() which ignores its > return value. Intentional, or missing error handling? Hi -- I was going through the unclassified Coverity issues again today, and this one's still on the list. Is this a bug, or intentional? thanks -- PMM
Re: [PULL 05/27] hw/xen: Watches on XenStore transactions
On Tue, 7 Mar 2023 at 18:27, David Woodhouse wrote: > > From: David Woodhouse > > Firing watches on the nodes that still exist is relatively easy; just > walk the tree and look at the nodes with refcount of one. > > Firing watches on *deleted* nodes is more fun. We add 'modified_in_tx' > and 'deleted_in_tx' flags to each node. Nodes with those flags cannot > be shared, as they will always be unique to the transaction in which > they were created. > > When xs_node_walk would need to *create* a node as scaffolding and it > encounters a deleted_in_tx node, it can resurrect it simply by clearing > its deleted_in_tx flag. If that node originally had any *data*, they're > gone, and the modified_in_tx flag will have been set when it was first > deleted. > > We then attempt to send appropriate watches when the transaction is > committed, properly delete the deleted_in_tx nodes, and remove the > modified_in_tx flag from the others. > > Signed-off-by: David Woodhouse > Reviewed-by: Paul Durrant Hi; Coverity's "is there missing error handling?" heuristic fired for a change in this code (CID 1508359): > static int transaction_commit(XenstoreImplState *s, XsTransaction *tx) > { > +struct walk_op op; > +XsNode **n; > + > if (s->root_tx != tx->base_tx) { > return EAGAIN; > } > @@ -720,10 +861,18 @@ static int transaction_commit(XenstoreImplState *s, > XsTransaction *tx) > s->root_tx = tx->tx_id; > s->nr_nodes = tx->nr_nodes; > > +init_walk_op(s, , XBT_NULL, tx->dom_id, "/", ); This is the only call to init_walk_op() which ignores its return value. Intentional, or missing error handling? > +op.deleted_in_tx = false; > +op.mutating = true; > + > /* > - * XX: Walk the new root and fire watches on any node which has a > + * Walk the new root and fire watches on any node which has a > * refcount of one (which is therefore unique to this transaction). > */ > +if (s->root->children) { > +g_hash_table_foreach_remove(s->root->children, tx_commit_walk, ); > +} > + > return 0; > } thanks -- PMM
[PULL 05/27] hw/xen: Watches on XenStore transactions
From: David Woodhouse Firing watches on the nodes that still exist is relatively easy; just walk the tree and look at the nodes with refcount of one. Firing watches on *deleted* nodes is more fun. We add 'modified_in_tx' and 'deleted_in_tx' flags to each node. Nodes with those flags cannot be shared, as they will always be unique to the transaction in which they were created. When xs_node_walk would need to *create* a node as scaffolding and it encounters a deleted_in_tx node, it can resurrect it simply by clearing its deleted_in_tx flag. If that node originally had any *data*, they're gone, and the modified_in_tx flag will have been set when it was first deleted. We then attempt to send appropriate watches when the transaction is committed, properly delete the deleted_in_tx nodes, and remove the modified_in_tx flag from the others. Signed-off-by: David Woodhouse Reviewed-by: Paul Durrant --- hw/i386/kvm/xenstore_impl.c | 151 ++- tests/unit/test-xs-node.c | 231 +++- 2 files changed, 380 insertions(+), 2 deletions(-) diff --git a/hw/i386/kvm/xenstore_impl.c b/hw/i386/kvm/xenstore_impl.c index 0812e367b0..60f42f61d6 100644 --- a/hw/i386/kvm/xenstore_impl.c +++ b/hw/i386/kvm/xenstore_impl.c @@ -32,6 +32,8 @@ typedef struct XsNode { GByteArray *content; GHashTable *children; uint64_t gencnt; +bool deleted_in_tx; +bool modified_in_tx; #ifdef XS_NODE_UNIT_TEST gchar *name; /* debug only */ #endif @@ -153,6 +155,13 @@ static XsNode *xs_node_copy(XsNode *old) XsNode *n = xs_node_new(); n->gencnt = old->gencnt; + +#ifdef XS_NODE_UNIT_TEST +if (n->name) { +n->name = g_strdup(old->name); +} +#endif + if (old->children) { n->children = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, (GDestroyNotify)xs_node_unref); @@ -221,6 +230,9 @@ struct walk_op { bool mutating; bool create_dirs; bool in_transaction; + +/* Tracking during recursion so we know which is first. */ +bool deleted_in_tx; }; static void fire_watches(struct walk_op *op, bool parents) @@ -277,6 +289,9 @@ static int xs_node_add_content(XsNode **n, struct walk_op *op) g_byte_array_unref((*n)->content); } (*n)->content = g_byte_array_ref(data); +if (op->tx_id != XBT_NULL) { +(*n)->modified_in_tx = true; +} return 0; } @@ -333,10 +348,62 @@ static int node_rm_recurse(gpointer key, gpointer value, gpointer user_data) return this_inplace; } +static XsNode *xs_node_copy_deleted(XsNode *old, struct walk_op *op); +static void copy_deleted_recurse(gpointer key, gpointer value, + gpointer user_data) +{ +struct walk_op *op = user_data; +GHashTable *siblings = op->op_opaque2; +XsNode *n = xs_node_copy_deleted(value, op); + +/* + * Reinsert the deleted_in_tx copy of the node into the parent's + * 'children' hash table. Having stashed it from op->op_opaque2 + * before the recursive call to xs_node_copy_deleted() scribbled + * over it. + */ +g_hash_table_insert(siblings, g_strdup(key), n); +} + +static XsNode *xs_node_copy_deleted(XsNode *old, struct walk_op *op) +{ +XsNode *n = xs_node_new(); + +n->gencnt = old->gencnt; + +#ifdef XS_NODE_UNIT_TEST +if (old->name) { +n->name = g_strdup(old->name); +} +#endif + +if (old->children) { +n->children = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, +(GDestroyNotify)xs_node_unref); +op->op_opaque2 = n->children; +g_hash_table_foreach(old->children, copy_deleted_recurse, op); +} +n->deleted_in_tx = true; +/* If it gets resurrected we only fire a watch if it lost its content */ +if (old->content) { +n->modified_in_tx = true; +} +op->new_nr_nodes--; +return n; +} + static int xs_node_rm(XsNode **n, struct walk_op *op) { bool this_inplace = op->inplace; +if (op->tx_id != XBT_NULL) { +/* It's not trivial to do inplace handling for this one */ +XsNode *old = *n; +*n = xs_node_copy_deleted(old, op); +xs_node_unref(old); +return 0; +} + /* Fire watches for, and count, nodes in the subtree which get deleted */ if ((*n)->children) { g_hash_table_foreach_remove((*n)->children, node_rm_recurse, op); @@ -408,6 +475,10 @@ static int xs_node_walk(XsNode **n, struct walk_op *op) } if (child) { +if (child->deleted_in_tx) { +assert(child->ref == 1); +/* Cannot actually set child->deleted_in_tx = false until later */ +} xs_node_ref(child); /* * Now we own it too. But if we can modify inplace, that's going to @@ -475,6 +546,15 @@ static int xs_node_walk(XsNode **n, struct walk_op *op) xs_node_unref(old); }