| Message ID | 20230530091333.7678-5-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: drop TDB | expand |
Hi Juergen, On 30/05/2023 10:13, Juergen Gross wrote: > diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h > index f7cb035f26..7fc6d73e5a 100644 > --- a/tools/xenstore/xenstored_core.h > +++ b/tools/xenstore/xenstored_core.h > @@ -358,11 +358,12 @@ extern xengnttab_handle **xgt_handle; > > int remember_string(struct hashtable *hash, const char *str); > > +/* Data base access functions. */ > void set_tdb_key(const char *name, TDB_DATA *key); > int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data, > struct node_account_data *acc, int flag, bool no_quota_check); > -int do_tdb_delete(struct connection *conn, TDB_DATA *key, > - struct node_account_data *acc); > +int db_delete(struct connection *conn, const char *name, > + struct node_account_data *acc); > > void conn_free_buffered_data(struct connection *conn); > > diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c > index 1646c07040..bf173f3d1d 100644 > --- a/tools/xenstore/xenstored_transaction.c > +++ b/tools/xenstore/xenstored_transaction.c > @@ -385,8 +385,7 @@ static int finalize_transaction(struct connection *conn, > /* Entries for unmodified nodes can be removed early. */ > if (!i->modified) { > if (i->ta_node) { > - set_tdb_key(i->trans_name, &ta_key); > - if (do_tdb_delete(conn, &ta_key, NULL)) > + if (db_delete(conn, i->trans_name, NULL)) > return EIO; > } > list_del(&i->list); > @@ -395,21 +394,21 @@ static int finalize_transaction(struct connection *conn, > } > > while ((i = list_top(&trans->accessed, struct accessed_node, list))) { > - set_tdb_key(i->node, &key); It is not clear to me why you are moving later the call to set_tdb_key() in this patch. > if (i->ta_node) { > set_tdb_key(i->trans_name, &ta_key); > data = tdb_fetch(tdb_ctx, ta_key); > if (data.dptr) { > - trace_tdb("read %s size %zu\n", ta_key.dptr, > + trace_tdb("read %s size %zu\n", i->trans_name, This change seems unrelated to this patch. > ta_key.dsize + data.dsize); > hdr = (void *)data.dptr; > hdr->generation = ++generation; > flag = (i->generation == NO_GENERATION) > ? NODE_CREATE : NODE_MODIFY; > + set_tdb_key(i->node, &key); > *is_corrupt |= do_tdb_write(conn, &key, &data, > NULL, flag, true); > talloc_free(data.dptr); > - if (do_tdb_delete(conn, &ta_key, NULL)) > + if (db_delete(conn, i->trans_name, NULL)) > *is_corrupt = true; > } else { > *is_corrupt = true; > @@ -422,7 +421,7 @@ static int finalize_transaction(struct connection *conn, > */ > *is_corrupt |= (i->generation == NO_GENERATION) > ? false > - : do_tdb_delete(conn, &key, NULL); > + : db_delete(conn, i->node, NULL); > } > if (i->fire_watch) > fire_watches(conn, trans, i->node, NULL, i->watch_exact, > @@ -439,15 +438,12 @@ static int destroy_transaction(void *_transaction) > { > struct transaction *trans = _transaction; > struct accessed_node *i; > - TDB_DATA key; > > wrl_ntransactions--; > trace_destroy(trans, "transaction"); > while ((i = list_top(&trans->accessed, struct accessed_node, list))) { > - if (i->ta_node) { > - set_tdb_key(i->trans_name, &key); > - do_tdb_delete(trans->conn, &key, NULL); > - } > + if (i->ta_node) > + db_delete(trans->conn, i->trans_name, NULL); > list_del(&i->list); > talloc_free(i); > } Cheers,
On 20.06.23 13:24, Julien Grall wrote: > Hi Juergen, > > On 30/05/2023 10:13, Juergen Gross wrote: >> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h >> index f7cb035f26..7fc6d73e5a 100644 >> --- a/tools/xenstore/xenstored_core.h >> +++ b/tools/xenstore/xenstored_core.h >> @@ -358,11 +358,12 @@ extern xengnttab_handle **xgt_handle; >> int remember_string(struct hashtable *hash, const char *str); >> +/* Data base access functions. */ >> void set_tdb_key(const char *name, TDB_DATA *key); >> int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data, >> struct node_account_data *acc, int flag, bool no_quota_check); >> -int do_tdb_delete(struct connection *conn, TDB_DATA *key, >> - struct node_account_data *acc); >> +int db_delete(struct connection *conn, const char *name, >> + struct node_account_data *acc); >> void conn_free_buffered_data(struct connection *conn); >> diff --git a/tools/xenstore/xenstored_transaction.c >> b/tools/xenstore/xenstored_transaction.c >> index 1646c07040..bf173f3d1d 100644 >> --- a/tools/xenstore/xenstored_transaction.c >> +++ b/tools/xenstore/xenstored_transaction.c >> @@ -385,8 +385,7 @@ static int finalize_transaction(struct connection *conn, >> /* Entries for unmodified nodes can be removed early. */ >> if (!i->modified) { >> if (i->ta_node) { >> - set_tdb_key(i->trans_name, &ta_key); >> - if (do_tdb_delete(conn, &ta_key, NULL)) >> + if (db_delete(conn, i->trans_name, NULL)) >> return EIO; >> } >> list_del(&i->list); >> @@ -395,21 +394,21 @@ static int finalize_transaction(struct connection *conn, >> } >> while ((i = list_top(&trans->accessed, struct accessed_node, list))) { >> - set_tdb_key(i->node, &key); > > It is not clear to me why you are moving later the call to set_tdb_key() in this > patch. It is needed in the if () case only now, before the patch the else case needed it, too. > >> if (i->ta_node) { >> set_tdb_key(i->trans_name, &ta_key); >> data = tdb_fetch(tdb_ctx, ta_key); >> if (data.dptr) { >> - trace_tdb("read %s size %zu\n", ta_key.dptr, >> + trace_tdb("read %s size %zu\n", i->trans_name, > > This change seems unrelated to this patch. Hmm, yes. I'll move that change to the patch where it belongs. Juergen
Hi Juergen, On 29/06/2023 10:29, Juergen Gross wrote: > On 20.06.23 13:24, Julien Grall wrote: >> Hi Juergen, >> >> On 30/05/2023 10:13, Juergen Gross wrote: >>> diff --git a/tools/xenstore/xenstored_core.h >>> b/tools/xenstore/xenstored_core.h >>> index f7cb035f26..7fc6d73e5a 100644 >>> --- a/tools/xenstore/xenstored_core.h >>> +++ b/tools/xenstore/xenstored_core.h >>> @@ -358,11 +358,12 @@ extern xengnttab_handle **xgt_handle; >>> int remember_string(struct hashtable *hash, const char *str); >>> +/* Data base access functions. */ >>> void set_tdb_key(const char *name, TDB_DATA *key); >>> int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA >>> *data, >>> struct node_account_data *acc, int flag, bool >>> no_quota_check); >>> -int do_tdb_delete(struct connection *conn, TDB_DATA *key, >>> - struct node_account_data *acc); >>> +int db_delete(struct connection *conn, const char *name, >>> + struct node_account_data *acc); >>> void conn_free_buffered_data(struct connection *conn); >>> diff --git a/tools/xenstore/xenstored_transaction.c >>> b/tools/xenstore/xenstored_transaction.c >>> index 1646c07040..bf173f3d1d 100644 >>> --- a/tools/xenstore/xenstored_transaction.c >>> +++ b/tools/xenstore/xenstored_transaction.c >>> @@ -385,8 +385,7 @@ static int finalize_transaction(struct connection >>> *conn, >>> /* Entries for unmodified nodes can be removed early. */ >>> if (!i->modified) { >>> if (i->ta_node) { >>> - set_tdb_key(i->trans_name, &ta_key); >>> - if (do_tdb_delete(conn, &ta_key, NULL)) >>> + if (db_delete(conn, i->trans_name, NULL)) >>> return EIO; >>> } >>> list_del(&i->list); >>> @@ -395,21 +394,21 @@ static int finalize_transaction(struct >>> connection *conn, >>> } >>> while ((i = list_top(&trans->accessed, struct accessed_node, >>> list))) { >>> - set_tdb_key(i->node, &key); >> >> It is not clear to me why you are moving later the call to >> set_tdb_key() in this patch. > > It is needed in the if () case only now, before the patch the else case > needed it, too. If that's the case, then can we also move the declaration to within the if case? This would make a bit more obvious that the other branch is not meant to use the variable anymore Cheers,
On 29.06.23 12:21, Julien Grall wrote: > Hi Juergen, > > On 29/06/2023 10:29, Juergen Gross wrote: >> On 20.06.23 13:24, Julien Grall wrote: >>> Hi Juergen, >>> >>> On 30/05/2023 10:13, Juergen Gross wrote: >>>> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h >>>> index f7cb035f26..7fc6d73e5a 100644 >>>> --- a/tools/xenstore/xenstored_core.h >>>> +++ b/tools/xenstore/xenstored_core.h >>>> @@ -358,11 +358,12 @@ extern xengnttab_handle **xgt_handle; >>>> int remember_string(struct hashtable *hash, const char *str); >>>> +/* Data base access functions. */ >>>> void set_tdb_key(const char *name, TDB_DATA *key); >>>> int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data, >>>> struct node_account_data *acc, int flag, bool no_quota_check); >>>> -int do_tdb_delete(struct connection *conn, TDB_DATA *key, >>>> - struct node_account_data *acc); >>>> +int db_delete(struct connection *conn, const char *name, >>>> + struct node_account_data *acc); >>>> void conn_free_buffered_data(struct connection *conn); >>>> diff --git a/tools/xenstore/xenstored_transaction.c >>>> b/tools/xenstore/xenstored_transaction.c >>>> index 1646c07040..bf173f3d1d 100644 >>>> --- a/tools/xenstore/xenstored_transaction.c >>>> +++ b/tools/xenstore/xenstored_transaction.c >>>> @@ -385,8 +385,7 @@ static int finalize_transaction(struct connection *conn, >>>> /* Entries for unmodified nodes can be removed early. */ >>>> if (!i->modified) { >>>> if (i->ta_node) { >>>> - set_tdb_key(i->trans_name, &ta_key); >>>> - if (do_tdb_delete(conn, &ta_key, NULL)) >>>> + if (db_delete(conn, i->trans_name, NULL)) >>>> return EIO; >>>> } >>>> list_del(&i->list); >>>> @@ -395,21 +394,21 @@ static int finalize_transaction(struct connection *conn, >>>> } >>>> while ((i = list_top(&trans->accessed, struct accessed_node, list))) { >>>> - set_tdb_key(i->node, &key); >>> >>> It is not clear to me why you are moving later the call to set_tdb_key() in >>> this patch. >> >> It is needed in the if () case only now, before the patch the else case >> needed it, too. > > If that's the case, then can we also move the declaration to within the if case? > This would make a bit more obvious that the other branch is not meant to use the > variable anymore No, key is used in the previous loop, too. Juergen
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index 239f8c6bc4..a2454ad24d 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -657,28 +657,31 @@ int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data, return 0; } -int do_tdb_delete(struct connection *conn, TDB_DATA *key, - struct node_account_data *acc) +int db_delete(struct connection *conn, const char *name, + struct node_account_data *acc) { struct node_account_data tmp_acc; unsigned int domid; + TDB_DATA key; + + set_tdb_key(name, &key); if (!acc) { acc = &tmp_acc; acc->memory = -1; } - get_acc_data(key, acc); + get_acc_data(&key, acc); - if (tdb_delete(tdb_ctx, *key)) { + if (tdb_delete(tdb_ctx, key)) { errno = EIO; return errno; } - trace_tdb("delete %s\n", key->dptr); + trace_tdb("delete %s\n", name); if (acc->memory) { - domid = get_acc_domid(conn, key, acc->domid); - domain_memory_add_nochk(conn, domid, -acc->memory - key->dsize); + domid = get_acc_domid(conn, &key, acc->domid); + domain_memory_add_nochk(conn, domid, -acc->memory - key.dsize); } return 0; @@ -1449,13 +1452,10 @@ nomem: static void destroy_node_rm(struct connection *conn, struct node *node) { - TDB_DATA key; - if (streq(node->name, "/")) corrupt(NULL, "Destroying root node!"); - set_tdb_key(node->db_name, &key); - do_tdb_delete(conn, &key, &node->acc); + db_delete(conn, node->db_name, &node->acc); } static int destroy_node(struct connection *conn, struct node *node) @@ -1646,7 +1646,6 @@ static int delnode_sub(const void *ctx, struct connection *conn, bool watch_exact; int ret; const char *db_name; - TDB_DATA key; /* Any error here will probably be repeated for all following calls. */ ret = access_node(conn, node, NODE_ACCESS_DELETE, &db_name); @@ -1657,8 +1656,7 @@ static int delnode_sub(const void *ctx, struct connection *conn, return WALK_TREE_ERROR_STOP; /* In case of error stop the walk. */ - set_tdb_key(db_name, &key); - if (!ret && do_tdb_delete(conn, &key, &node->acc)) + if (!ret && db_delete(conn, db_name, &node->acc)) return WALK_TREE_ERROR_STOP; /* @@ -2483,9 +2481,8 @@ static int clean_store_(TDB_CONTEXT *tdb, TDB_DATA key, TDB_DATA val, } if (!hashtable_search(reachable, name)) { log("clean_store: '%s' is orphaned!", name); - if (recovery) { - do_tdb_delete(NULL, &key, NULL); - } + if (recovery) + db_delete(NULL, name, NULL); } talloc_free(name); diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h index f7cb035f26..7fc6d73e5a 100644 --- a/tools/xenstore/xenstored_core.h +++ b/tools/xenstore/xenstored_core.h @@ -358,11 +358,12 @@ extern xengnttab_handle **xgt_handle; int remember_string(struct hashtable *hash, const char *str); +/* Data base access functions. */ void set_tdb_key(const char *name, TDB_DATA *key); int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data, struct node_account_data *acc, int flag, bool no_quota_check); -int do_tdb_delete(struct connection *conn, TDB_DATA *key, - struct node_account_data *acc); +int db_delete(struct connection *conn, const char *name, + struct node_account_data *acc); void conn_free_buffered_data(struct connection *conn); diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c index 1646c07040..bf173f3d1d 100644 --- a/tools/xenstore/xenstored_transaction.c +++ b/tools/xenstore/xenstored_transaction.c @@ -385,8 +385,7 @@ static int finalize_transaction(struct connection *conn, /* Entries for unmodified nodes can be removed early. */ if (!i->modified) { if (i->ta_node) { - set_tdb_key(i->trans_name, &ta_key); - if (do_tdb_delete(conn, &ta_key, NULL)) + if (db_delete(conn, i->trans_name, NULL)) return EIO; } list_del(&i->list); @@ -395,21 +394,21 @@ static int finalize_transaction(struct connection *conn, } while ((i = list_top(&trans->accessed, struct accessed_node, list))) { - set_tdb_key(i->node, &key); if (i->ta_node) { set_tdb_key(i->trans_name, &ta_key); data = tdb_fetch(tdb_ctx, ta_key); if (data.dptr) { - trace_tdb("read %s size %zu\n", ta_key.dptr, + trace_tdb("read %s size %zu\n", i->trans_name, ta_key.dsize + data.dsize); hdr = (void *)data.dptr; hdr->generation = ++generation; flag = (i->generation == NO_GENERATION) ? NODE_CREATE : NODE_MODIFY; + set_tdb_key(i->node, &key); *is_corrupt |= do_tdb_write(conn, &key, &data, NULL, flag, true); talloc_free(data.dptr); - if (do_tdb_delete(conn, &ta_key, NULL)) + if (db_delete(conn, i->trans_name, NULL)) *is_corrupt = true; } else { *is_corrupt = true; @@ -422,7 +421,7 @@ static int finalize_transaction(struct connection *conn, */ *is_corrupt |= (i->generation == NO_GENERATION) ? false - : do_tdb_delete(conn, &key, NULL); + : db_delete(conn, i->node, NULL); } if (i->fire_watch) fire_watches(conn, trans, i->node, NULL, i->watch_exact, @@ -439,15 +438,12 @@ static int destroy_transaction(void *_transaction) { struct transaction *trans = _transaction; struct accessed_node *i; - TDB_DATA key; wrl_ntransactions--; trace_destroy(trans, "transaction"); while ((i = list_top(&trans->accessed, struct accessed_node, list))) { - if (i->ta_node) { - set_tdb_key(i->trans_name, &key); - do_tdb_delete(trans->conn, &key, NULL); - } + if (i->ta_node) + db_delete(trans->conn, i->trans_name, NULL); list_del(&i->list); talloc_free(i); }
Rename do_tdb_delete() to db_delete() and replace the key parameter with db_name specifying the name of the node in the data base. This is in preparation to replace TDB with a more simple data storage. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenstore/xenstored_core.c | 31 ++++++++++++-------------- tools/xenstore/xenstored_core.h | 5 +++-- tools/xenstore/xenstored_transaction.c | 18 ++++++--------- 3 files changed, 24 insertions(+), 30 deletions(-)