Re: [RFC PATCH v1 03/25] hw/xen: Implement XenStore watches
On Tue, 2023-03-07 at 11:29 +, Paul Durrant wrote:
>
> 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.
Good point. The EEXIST check was added later as I was reviewing and
comparing with the real xenstored, so I didn't spot that. Thanks.
--- a/hw/i386/kvm/xenstore_impl.c
+++ b/hw/i386/kvm/xenstore_impl.c
@@ -673,7 +673,7 @@ int xs_impl_watch(XenstoreImplState *s, unsigned int
dom_id, const char *path,
}
/* Check for duplicates */
-w = g_hash_table_lookup(s->watches, abspath);
+l = 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) {
@@ -693,7 +693,7 @@ int xs_impl_watch(XenstoreImplState *s, unsigned int
dom_id, const char *path,
w->dom_id = dom_id;
w->rel_prefix = strlen(abspath) - strlen(path);
-l = g_hash_table_lookup(s->watches, abspath);
+/* l was looked up above when checking for duplicates */
if (l) {
w->next = l->next;
l->next = w;
smime.p7s
Description: S/MIME cryptographic signature
Re: [RFC PATCH v1 03/25] hw/xen: Implement XenStore watches
On 02/03/2023 15:34, David Woodhouse wrote:
From: David Woodhouse
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
---
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
... 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;
+}
