Re: [RFC PATCH v1 05/25] hw/xen: Watches on XenStore transactions

[Paul Durrant <xadimgnik@gmail.com>]

On 02/03/2023 15:34, David Woodhouse 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>
> ---
>   hw/i386/kvm/xenstore_impl.c | 151 ++++++++++++++++++++++-
>   tests/unit/test-xs-node.c   | 231 +++++++++++++++++++++++++++++++++++-
>   2 files changed, 380 insertions(+), 2 deletions(-)
> 

Reviewed-by: Paul Durrant <paul@xen.org>

... with a couple of nits in comments called out below...

[snip]
> +static gboolean tx_commit_walk(gpointer key, gpointer value,
> +                               gpointer user_data)
> +{
> +    struct walk_op *op = user_data;
> +    int path_len = strlen(op->path);
> +    int key_len = strlen(key);
> +    bool fire_parents = true;
> +    XsWatch *watch;
> +    XsNode *n = value;
> +
> +    if (n->ref != 1) {
> +        return false;
> +    }
> +
> +    if (n->deleted_in_tx) {
> +        /*
> +         * We first watches on our parents if we are the *first* node

We first *fire* watches on our parents...

> +         * to be deleted (the topmost one). This matches the behaviour
> +         * when deleting in the live tree.
> +         */
> +        fire_parents = !op->deleted_in_tx;
> +
> +        /* Only used on the way down so no need to clear it later */
> +        op->deleted_in_tx = true;
> +    }
> +
> +    assert(key_len + path_len + 2 <= sizeof(op->path));
> +    op->path[path_len] = '/';
> +    memcpy(op->path + path_len + 1, key, key_len + 1);
> +
> +    watch = g_hash_table_lookup(op->s->watches, op->path);
> +    if (watch) {
> +        op->watches = g_list_append(op->watches, watch);
> +    }
> +
> +    if (n->children) {
> +        g_hash_table_foreach_remove(n->children, tx_commit_walk, op);
> +    }
> +
> +    if (watch) {
> +        op->watches = g_list_remove(op->watches, watch);
> +    }
> +
> +    /*
> +     * Don't fire watches if this node was only copied because a
> +     * descendent was changed. The modifieD_in_tx flag indicates the

s/modifieD/modified

> +     * ones which were really changed.
> +     */
> +    if (n->modified_in_tx || n->deleted_in_tx) {
> +        fire_watches(op, fire_parents);
> +        n->modified_in_tx = false;
> +    }
> +    op->path[path_len] = '\0';
> +
> +    /* Deleted nodes really do get expunged when we commit */
> +    return n->deleted_in_tx;
> +}