Re: [PULL 05/27] hw/xen: Watches on XenStore transactions

[Peter Maydell]

On Tue, 7 Mar 2023 at 18:27, David Woodhouse <dwmw2@infradead.org> wrote:
>
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> 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 <dwmw@amazon.co.uk>
> Reviewed-by: Paul Durrant <paul@xen.org>

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, &op, XBT_NULL, tx->dom_id, "/", &n);

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, &op);
> +    }
> +
>      return 0;
>  }

thanks
-- PMM