| Message ID | 20230117091124.22170-17-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: do some cleanup and fixes | expand |
Hi Juergen, On 17/01/2023 09:11, Juergen Gross wrote: > Today check_store() is only testing the correctness of the node tree. > > Add verification of the accounting data (number of nodes) and correct NIT: one too many space before 'and'. > the data if it is wrong. > > Do the initial check_store() call only after Xenstore entries of a > live update have been read. Can you clarify whether this is needed for the rest of the patch, or simply a nice thing to have in general? > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > tools/xenstore/xenstored_core.c | 62 ++++++++++++++++------ > tools/xenstore/xenstored_domain.c | 86 +++++++++++++++++++++++++++++++ > tools/xenstore/xenstored_domain.h | 4 ++ > 3 files changed, 137 insertions(+), 15 deletions(-) > > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c > index 3099077a86..e201f14053 100644 > --- a/tools/xenstore/xenstored_core.c > +++ b/tools/xenstore/xenstored_core.c > @@ -2389,8 +2389,6 @@ void setup_structure(bool live_update) > manual_node("@introduceDomain", NULL); > domain_nbentry_fix(dom0_domid, 5, true); > } > - > - check_store(); > } > > static unsigned int hash_from_key_fn(void *k) > @@ -2433,20 +2431,28 @@ int remember_string(struct hashtable *hash, const char *str) > * As we go, we record each node in the given reachable hashtable. These > * entries will be used later in clean_store. > */ > + > +struct check_store_data { > + struct hashtable *reachable; > + struct hashtable *domains; > +}; > + > static int check_store_step(const void *ctx, struct connection *conn, > struct node *node, void *arg) > { > - struct hashtable *reachable = arg; > + struct check_store_data *data = arg; > > - if (hashtable_search(reachable, (void *)node->name)) { > + if (hashtable_search(data->reachable, (void *)node->name)) { IIUC the cast is only necessary because hashtable_search() expects a non-const value. I can't think for a reason for the key to be non-const. So I will look to send a follow-up patch. > + > +void domain_check_acc_add(const struct node *node, struct hashtable *domains) > +{ > + struct domain_acc *dom; > + unsigned int domid; > + > + domid = node->perms.p[0].id; This could be replaced with get_node_owner(). > + dom = hashtable_search(domains, &domid); > + if (!dom) > + log("Node %s owned by unknown domain %u", node->name, domid); > + else > + dom->nodes++; > +} > + > +static int domain_check_acc_sub(const void *k, void *v, void *arg) I find the suffix 'sub' misleading because we have a function with a very similar name (instead suffixed 'sub'). Yet, AFAICT, it is not meant to substract. So I would prefix with '_cb' instead. Cheers,
On 17.01.23 23:36, Julien Grall wrote: > Hi Juergen, > > On 17/01/2023 09:11, Juergen Gross wrote: >> Today check_store() is only testing the correctness of the node tree. >> >> Add verification of the accounting data (number of nodes) and correct > > NIT: one too many space before 'and'. > >> the data if it is wrong. >> >> Do the initial check_store() call only after Xenstore entries of a >> live update have been read. > > Can you clarify whether this is needed for the rest of the patch, or simply a > nice thing to have in general? I'll add: "This is wanted to make sure the accounting data is correct after a live update." > >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> tools/xenstore/xenstored_core.c | 62 ++++++++++++++++------ >> tools/xenstore/xenstored_domain.c | 86 +++++++++++++++++++++++++++++++ >> tools/xenstore/xenstored_domain.h | 4 ++ >> 3 files changed, 137 insertions(+), 15 deletions(-) >> >> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c >> index 3099077a86..e201f14053 100644 >> --- a/tools/xenstore/xenstored_core.c >> +++ b/tools/xenstore/xenstored_core.c >> @@ -2389,8 +2389,6 @@ void setup_structure(bool live_update) >> manual_node("@introduceDomain", NULL); >> domain_nbentry_fix(dom0_domid, 5, true); >> } >> - >> - check_store(); >> } >> static unsigned int hash_from_key_fn(void *k) >> @@ -2433,20 +2431,28 @@ int remember_string(struct hashtable *hash, const char >> *str) >> * As we go, we record each node in the given reachable hashtable. These >> * entries will be used later in clean_store. >> */ >> + >> +struct check_store_data { >> + struct hashtable *reachable; >> + struct hashtable *domains; >> +}; >> + >> static int check_store_step(const void *ctx, struct connection *conn, >> struct node *node, void *arg) >> { >> - struct hashtable *reachable = arg; >> + struct check_store_data *data = arg; >> - if (hashtable_search(reachable, (void *)node->name)) { >> + if (hashtable_search(data->reachable, (void *)node->name)) { > > IIUC the cast is only necessary because hashtable_search() expects a non-const > value. I can't think for a reason for the key to be non-const. So I will look to > send a follow-up patch. It is possible, but nasty, due to talloc_free() not taking a const pointer. > >> + >> +void domain_check_acc_add(const struct node *node, struct hashtable *domains) >> +{ >> + struct domain_acc *dom; >> + unsigned int domid; >> + >> + domid = node->perms.p[0].id; > > This could be replaced with get_node_owner(). Indeed. > >> + dom = hashtable_search(domains, &domid); >> + if (!dom) >> + log("Node %s owned by unknown domain %u", node->name, domid); >> + else >> + dom->nodes++; >> +} >> + >> +static int domain_check_acc_sub(const void *k, void *v, void *arg) > > I find the suffix 'sub' misleading because we have a function with a very > similar name (instead suffixed 'sub'). Yet, AFAICT, it is not meant to substract. > > So I would prefix with '_cb' instead. Fine with me. Juergen
Hi Juergen, On 18/01/2023 06:23, Juergen Gross wrote: > On 17.01.23 23:36, Julien Grall wrote: >> Hi Juergen, >> >> On 17/01/2023 09:11, Juergen Gross wrote: >>> Today check_store() is only testing the correctness of the node tree. >>> >>> Add verification of the accounting data (number of nodes) and correct >> >> NIT: one too many space before 'and'. >> >>> the data if it is wrong. >>> >>> Do the initial check_store() call only after Xenstore entries of a >>> live update have been read. >> >> Can you clarify whether this is needed for the rest of the patch, or >> simply a nice thing to have in general? > > I'll add: "This is wanted to make sure the accounting data is correct after > a live update." Fine with me. > >> >>> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> --- >>> tools/xenstore/xenstored_core.c | 62 ++++++++++++++++------ >>> tools/xenstore/xenstored_domain.c | 86 +++++++++++++++++++++++++++++++ >>> tools/xenstore/xenstored_domain.h | 4 ++ >>> 3 files changed, 137 insertions(+), 15 deletions(-) >>> >>> diff --git a/tools/xenstore/xenstored_core.c >>> b/tools/xenstore/xenstored_core.c >>> index 3099077a86..e201f14053 100644 >>> --- a/tools/xenstore/xenstored_core.c >>> +++ b/tools/xenstore/xenstored_core.c >>> @@ -2389,8 +2389,6 @@ void setup_structure(bool live_update) >>> manual_node("@introduceDomain", NULL); >>> domain_nbentry_fix(dom0_domid, 5, true); >>> } >>> - >>> - check_store(); >>> } >>> static unsigned int hash_from_key_fn(void *k) >>> @@ -2433,20 +2431,28 @@ int remember_string(struct hashtable *hash, >>> const char *str) >>> * As we go, we record each node in the given reachable hashtable. >>> These >>> * entries will be used later in clean_store. >>> */ >>> + >>> +struct check_store_data { >>> + struct hashtable *reachable; >>> + struct hashtable *domains; >>> +}; >>> + >>> static int check_store_step(const void *ctx, struct connection *conn, >>> struct node *node, void *arg) >>> { >>> - struct hashtable *reachable = arg; >>> + struct check_store_data *data = arg; >>> - if (hashtable_search(reachable, (void *)node->name)) { >>> + if (hashtable_search(data->reachable, (void *)node->name)) { >> >> IIUC the cast is only necessary because hashtable_search() expects a >> non-const value. I can't think for a reason for the key to be >> non-const. So I will look to send a follow-up patch. > > It is possible, but nasty, due to talloc_free() not taking a const pointer. I am not sure I understand your reasoning. Looking at hashtable_search(), I don't see a call to talloc_free(). Anyway, this is not directly related to this patch. So I will have a look separately. Cheers,
On 18.01.23 10:35, Julien Grall wrote: > Hi Juergen, > > On 18/01/2023 06:23, Juergen Gross wrote: >> On 17.01.23 23:36, Julien Grall wrote: >>> Hi Juergen, >>> >>> On 17/01/2023 09:11, Juergen Gross wrote: >>>> Today check_store() is only testing the correctness of the node tree. >>>> >>>> Add verification of the accounting data (number of nodes) and correct >>> >>> NIT: one too many space before 'and'. >>> >>>> the data if it is wrong. >>>> >>>> Do the initial check_store() call only after Xenstore entries of a >>>> live update have been read. >>> >>> Can you clarify whether this is needed for the rest of the patch, or simply a >>> nice thing to have in general? >> >> I'll add: "This is wanted to make sure the accounting data is correct after >> a live update." > > Fine with me. > >> >>> >>>> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>> --- >>>> tools/xenstore/xenstored_core.c | 62 ++++++++++++++++------ >>>> tools/xenstore/xenstored_domain.c | 86 +++++++++++++++++++++++++++++++ >>>> tools/xenstore/xenstored_domain.h | 4 ++ >>>> 3 files changed, 137 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c >>>> index 3099077a86..e201f14053 100644 >>>> --- a/tools/xenstore/xenstored_core.c >>>> +++ b/tools/xenstore/xenstored_core.c >>>> @@ -2389,8 +2389,6 @@ void setup_structure(bool live_update) >>>> manual_node("@introduceDomain", NULL); >>>> domain_nbentry_fix(dom0_domid, 5, true); >>>> } >>>> - >>>> - check_store(); >>>> } >>>> static unsigned int hash_from_key_fn(void *k) >>>> @@ -2433,20 +2431,28 @@ int remember_string(struct hashtable *hash, const >>>> char *str) >>>> * As we go, we record each node in the given reachable hashtable. These >>>> * entries will be used later in clean_store. >>>> */ >>>> + >>>> +struct check_store_data { >>>> + struct hashtable *reachable; >>>> + struct hashtable *domains; >>>> +}; >>>> + >>>> static int check_store_step(const void *ctx, struct connection *conn, >>>> struct node *node, void *arg) >>>> { >>>> - struct hashtable *reachable = arg; >>>> + struct check_store_data *data = arg; >>>> - if (hashtable_search(reachable, (void *)node->name)) { >>>> + if (hashtable_search(data->reachable, (void *)node->name)) { >>> >>> IIUC the cast is only necessary because hashtable_search() expects a >>> non-const value. I can't think for a reason for the key to be non-const. So I >>> will look to send a follow-up patch. >> >> It is possible, but nasty, due to talloc_free() not taking a const pointer. > > I am not sure I understand your reasoning. Looking at hashtable_search(), I > don't see a call to talloc_free(). Oh, I thought you were referring to the key in general. Juergen
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index 3099077a86..e201f14053 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -2389,8 +2389,6 @@ void setup_structure(bool live_update) manual_node("@introduceDomain", NULL); domain_nbentry_fix(dom0_domid, 5, true); } - - check_store(); } static unsigned int hash_from_key_fn(void *k) @@ -2433,20 +2431,28 @@ int remember_string(struct hashtable *hash, const char *str) * As we go, we record each node in the given reachable hashtable. These * entries will be used later in clean_store. */ + +struct check_store_data { + struct hashtable *reachable; + struct hashtable *domains; +}; + static int check_store_step(const void *ctx, struct connection *conn, struct node *node, void *arg) { - struct hashtable *reachable = arg; + struct check_store_data *data = arg; - if (hashtable_search(reachable, (void *)node->name)) { + if (hashtable_search(data->reachable, (void *)node->name)) { log("check_store: '%s' is duplicated!", node->name); return recovery ? WALK_TREE_RM_CHILDENTRY : WALK_TREE_SKIP_CHILDREN; } - if (!remember_string(reachable, node->name)) + if (!remember_string(data->reachable, node->name)) return WALK_TREE_ERROR_STOP; + domain_check_acc_add(node, data->domains); + return WALK_TREE_OK; } @@ -2496,37 +2502,61 @@ static int clean_store_(TDB_CONTEXT *tdb, TDB_DATA key, TDB_DATA val, * Given the list of reachable nodes, iterate over the whole store, and * remove any that were not reached. */ -static void clean_store(struct hashtable *reachable) +static void clean_store(struct check_store_data *data) { - tdb_traverse(tdb_ctx, &clean_store_, reachable); + tdb_traverse(tdb_ctx, &clean_store_, data->reachable); + domain_check_acc(data->domains); } +int check_store_path(const char *name, struct check_store_data *data) +{ + struct node *node; + + node = read_node(NULL, NULL, name); + if (!node) { + log("check_store: error %d reading special node '%s'", errno, + name); + return errno; + } + + return check_store_step(NULL, NULL, node, data); +} void check_store(void) { - struct hashtable *reachable; struct walk_funcs walkfuncs = { .enter = check_store_step, .enoent = check_store_enoent, }; + struct check_store_data data; /* Don't free values (they are all void *1) */ - reachable = create_hashtable(NULL, 16, hash_from_key_fn, keys_equal_fn, - HASHTABLE_FREE_KEY); - if (!reachable) { + data.reachable = create_hashtable(NULL, 16, hash_from_key_fn, + keys_equal_fn, HASHTABLE_FREE_KEY); + if (!data.reachable) { log("check_store: ENOMEM"); return; } + data.domains = domain_check_acc_init(); + if (!data.domains) { + log("check_store: ENOMEM"); + goto out_hash; + } + log("Checking store ..."); - if (walk_node_tree(NULL, NULL, "/", &walkfuncs, reachable)) { + if (walk_node_tree(NULL, NULL, "/", &walkfuncs, &data)) { if (errno == ENOMEM) log("check_store: ENOMEM"); - } else if (!check_transactions(reachable)) - clean_store(reachable); + } else if (!check_store_path("@introduceDomain", &data) && + !check_store_path("@releaseDomain", &data) && + !check_transactions(data.reachable)) + clean_store(&data); log("Checking store complete."); - hashtable_destroy(reachable); + hashtable_destroy(data.domains); + out_hash: + hashtable_destroy(data.reachable); } @@ -2925,6 +2955,8 @@ int main(int argc, char *argv[]) lu_read_state(); #endif + check_store(); + /* Get ready to listen to the tools. */ initialize_fds(&sock_pollfd_idx, &timeout); diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c index cb1f09c297..a3c4fb7f93 100644 --- a/tools/xenstore/xenstored_domain.c +++ b/tools/xenstore/xenstored_domain.c @@ -1573,6 +1573,92 @@ void read_state_connection(const void *ctx, const void *state) read_state_buffered_data(ctx, conn, sc); } +struct domain_acc { + unsigned int domid; + int nodes; +}; + +static int domain_check_acc_init_sub(const void *k, void *v, void *arg) +{ + struct hashtable *domains = arg; + struct domain *d = v; + struct domain_acc *dom; + + dom = talloc_zero(NULL, struct domain_acc); + if (!dom) + return -1; + + dom->domid = d->domid; + /* + * Set the initial value to the negative one of the current domain. + * If everything is correct incrementing the value for each node will + * result in dom->nodes being 0 at the end. + */ + dom->nodes = -d->nbentry; + + if (!hashtable_insert(domains, &dom->domid, dom)) { + talloc_free(dom); + return -1; + } + + return 0; +} + +struct hashtable *domain_check_acc_init(void) +{ + struct hashtable *domains; + + domains = create_hashtable(NULL, 8, domhash_fn, domeq_fn, + HASHTABLE_FREE_VALUE); + if (!domains) + return NULL; + + if (hashtable_iterate(domhash, domain_check_acc_init_sub, domains)) { + hashtable_destroy(domains); + return NULL; + } + + return domains; +} + +void domain_check_acc_add(const struct node *node, struct hashtable *domains) +{ + struct domain_acc *dom; + unsigned int domid; + + domid = node->perms.p[0].id; + dom = hashtable_search(domains, &domid); + if (!dom) + log("Node %s owned by unknown domain %u", node->name, domid); + else + dom->nodes++; +} + +static int domain_check_acc_sub(const void *k, void *v, void *arg) +{ + struct domain_acc *dom = v; + struct domain *d; + + if (!dom->nodes) + return 0; + + log("Correct accounting data for domain %u: nodes are %d off", + dom->domid, dom->nodes); + + d = find_domain_struct(dom->domid); + if (!d) + return 0; + + d->nbentry += dom->nodes; + + return 0; +} + +void domain_check_acc(struct hashtable *domains) +{ + hashtable_iterate(domains, domain_check_acc_sub, NULL); +} + /* * Local variables: * mode: C diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h index 22996e2576..dc4660861e 100644 --- a/tools/xenstore/xenstored_domain.h +++ b/tools/xenstore/xenstored_domain.h @@ -129,4 +129,8 @@ const char *dump_state_connections(FILE *fp); void read_state_connection(const void *ctx, const void *state); +struct hashtable *domain_check_acc_init(void); +void domain_check_acc_add(const struct node *node, struct hashtable *domains); +void domain_check_acc(struct hashtable *domains); + #endif /* _XENSTORED_DOMAIN_H */
Today check_store() is only testing the correctness of the node tree. Add verification of the accounting data (number of nodes) and correct the data if it is wrong. Do the initial check_store() call only after Xenstore entries of a live update have been read. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenstore/xenstored_core.c | 62 ++++++++++++++++------ tools/xenstore/xenstored_domain.c | 86 +++++++++++++++++++++++++++++++ tools/xenstore/xenstored_domain.h | 4 ++ 3 files changed, 137 insertions(+), 15 deletions(-)