Re: [RFC PATCH v1 03/25] hw/xen: Implement XenStore watches

[Paul Durrant <xadimgnik@xxxxxxxxx>]

On 02/03/2023 15:34, David Woodhouse wrote:
> From: David Woodhouse <dwmw@xxxxxxxxxxxx>
>
> Starts out fairly simple: a hash table of watches based on the path.
>
> Except there can be multiple watches on the same path, so the watch ends
> up being a simple linked list, and the head of that list is in the hash
> table. Which makes removal a bit of a PITA but it's not so bad; we just
> special-case "I had to remove the head of the list and now I have to
> replace it in / remove it from the hash table". And if we don't remove
> the head, it's a simple linked-list operation.
>
> We do need to fire watches on *deleted* nodes, so instead of just a simple
> xs_node_unref() on the topmost victim, we need to recurse down and fire
> watches on them all.
>
> Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
> ---
>   hw/i386/kvm/xenstore_impl.c | 253 +++++++++++++++++++++++++++++++++---
>   tests/unit/test-xs-node.c   |  85 ++++++++++++
>   2 files changed, 323 insertions(+), 15 deletions(-)
Reviewed-by: Paul Durrant <paul@xxxxxxx>

... with one suggestion...

[snip]
> +    /* Check for duplicates */
> +    w = g_hash_table_lookup(s->watches, abspath);
> +    while (w) {
> +        if (!g_strcmp0(token, w->token) &&  opaque == w->cb_opaque &&
> +            fn == w->cb && dom_id == w->dom_id) {
> +            return EEXIST;
> +        }
> +        w = w->next;
I think you could stash a tail pointer here...

> +    }
> +
> +    if (dom_id && s->nr_domu_watches >= XS_MAX_WATCHES) {
> +        return E2BIG;
> +    }
> +
> +    w = g_new0(XsWatch, 1);
> +    w->token = g_strdup(token);
> +    w->cb = fn;
> +    w->cb_opaque = opaque;
> +    w->dom_id = dom_id;
> +    w->rel_prefix = strlen(abspath) - strlen(path);
> +
> +    l = g_hash_table_lookup(s->watches, abspath);
... to avoid the duplicate hash lookup here.

> +    if (l) {
> +        w->next = l->next;
> +        l->next = w;
> +    } else {
> +        g_hash_table_insert(s->watches, g_strdup(abspath), w);
> +    }
> +    if (dom_id) {
> +        s->nr_domu_watches++;
> +    }
> +
> +    /* A new watch should fire immediately */
> +    fn(opaque, path, token);
> +
> +    return 0;
> +}