| Message ID | 20230530091333.7678-2-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: > Instead of using TDB_REPLACE for either creating or modifying a TDB > entry, use either TDB_INSERT or TDB_MODIFY when calling tdb_store(). > > At higher function levels use the abstract flag values NODE_CREATE > and NODE_MODIFY. > > This is for preparing to get rid of TDB. IIUC, you want to make the change so don't need to check if the node exists when we are creating a new node. If so, isn't this change also beneficial for TDB? > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > tools/xenstore/xenstored_core.c | 29 ++++++++++++++------------ > tools/xenstore/xenstored_core.h | 6 ++++-- > tools/xenstore/xenstored_domain.c | 2 +- > tools/xenstore/xenstored_transaction.c | 8 +++++-- > 4 files changed, 27 insertions(+), 18 deletions(-) > > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c > index 31a862b715..90c0bc7423 100644 > --- a/tools/xenstore/xenstored_core.c > +++ b/tools/xenstore/xenstored_core.c > @@ -601,7 +601,7 @@ static unsigned int get_acc_domid(struct connection *conn, TDB_DATA *key, > } > > int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data, > - struct node_account_data *acc, bool no_quota_check) > + struct node_account_data *acc, int flag, bool no_quota_check) 'int' is the wrong type for a flag. Is flag only meant to have two values? If yes, then it would be best to switch to bool (with define if you still want explicit name). If not, then I think this should be an enum. Rather than bool, I would also be happy if you want enum with two values. > { > struct xs_tdb_record_hdr *hdr = (void *)data->dptr; > struct node_account_data old_acc = {}; > @@ -635,7 +635,8 @@ int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data, > } > > /* TDB should set errno, but doesn't even set ecode AFAICT. */ > - if (tdb_store(tdb_ctx, *key, *data, TDB_REPLACE) != 0) { > + if (tdb_store(tdb_ctx, *key, *data, > + (flag == NODE_CREATE) ? TDB_INSERT : TDB_MODIFY) != 0) { > domain_memory_add_nochk(conn, new_domid, > -data->dsize - key->dsize); > /* Error path, so no quota check. */ > @@ -774,7 +775,7 @@ static bool read_node_can_propagate_errno(void) > } > > int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node, > - bool no_quota_check) > + int flag, bool no_quota_check) > { > TDB_DATA data; > void *p; > @@ -812,7 +813,7 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node, > p += node->datalen; > memcpy(p, node->children, node->childlen); > > - if (do_tdb_write(conn, key, &data, &node->acc, no_quota_check)) > + if (do_tdb_write(conn, key, &data, &node->acc, flag, no_quota_check)) > return EIO; > > return 0; > @@ -823,14 +824,14 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node, > * node->key. This can later be used if the change needs to be reverted. > */ > static int write_node(struct connection *conn, struct node *node, > - bool no_quota_check) > + int flag, bool no_quota_check) > { > int ret; > > if (access_node(conn, node, NODE_ACCESS_WRITE, &node->key)) > return errno; > > - ret = write_node_raw(conn, &node->key, node, no_quota_check); > + ret = write_node_raw(conn, &node->key, node, flag, no_quota_check); > if (ret && conn && conn->transaction) { > /* > * Reverting access_node() is hard, so just fail the > @@ -1496,7 +1497,8 @@ static struct node *create_node(struct connection *conn, const void *ctx, > goto err; > } > > - ret = write_node(conn, i, false); > + ret = write_node(conn, i, i->parent ? NODE_CREATE : NODE_MODIFY, > + false); > if (ret) > goto err; > > @@ -1560,7 +1562,7 @@ static int do_write(const void *ctx, struct connection *conn, > } else { > node->data = in->buffer + offset; > node->datalen = datalen; > - if (write_node(conn, node, false)) > + if (write_node(conn, node, NODE_MODIFY, false)) > return errno; > } > > @@ -1610,7 +1612,7 @@ static int remove_child_entry(struct connection *conn, struct node *node, > memdel(node->children, offset, childlen + 1, node->childlen); > node->childlen -= childlen + 1; > > - return write_node(conn, node, true); > + return write_node(conn, node, NODE_MODIFY, true); > } > > static int delete_child(struct connection *conn, > @@ -1807,7 +1809,7 @@ static int do_set_perms(const void *ctx, struct connection *conn, > if (domain_nbentry_inc(conn, get_node_owner(node))) > return ENOMEM; > > - if (write_node(conn, node, false)) > + if (write_node(conn, node, NODE_MODIFY, false)) > return errno; > > fire_watches(conn, ctx, name, node, false, &old_perms); > @@ -2321,7 +2323,7 @@ static void manual_node(const char *name, const char *child) > if (child) > node->childlen = strlen(child) + 1; > > - if (write_node(NULL, node, false)) > + if (write_node(NULL, node, NODE_CREATE, false)) > barf_perror("Could not create initial node %s", name); > talloc_free(node); > } > @@ -3469,12 +3471,13 @@ void read_state_node(const void *ctx, const void *state) > barf("allocation error restoring node"); > > set_tdb_key(parentname, &key); > - if (write_node_raw(NULL, &key, parent, true)) > + if (write_node_raw(NULL, &key, parent, NODE_MODIFY, true)) > barf("write parent error restoring node"); > } > > set_tdb_key(name, &key); > - if (write_node_raw(NULL, &key, node, true)) > + if (write_node_raw(NULL, &key, node, > + strcmp(name, "/") ? NODE_CREATE : NODE_MODIFY, true)) I think a comment for the check would be useful. AFAUI, this is because "/" is always created before restoring the state. > barf("write node error restoring node"); > > if (domain_nbentry_inc(&conn, get_node_owner(node))) > diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h > index 84a611cbb5..9291efec17 100644 > --- a/tools/xenstore/xenstored_core.h > +++ b/tools/xenstore/xenstored_core.h > @@ -238,7 +238,9 @@ static inline unsigned int get_node_owner(const struct node *node) > > /* Write a node to the tdb data base. */ > int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node, > - bool no_quota_check); > + int flag, bool no_quota_check); > +#define NODE_CREATE 0 > +#define NODE_MODIFY 1 > > /* Get a node from the tdb data base. */ > struct node *read_node(struct connection *conn, const void *ctx, > @@ -358,7 +360,7 @@ int remember_string(struct hashtable *hash, const char *str); > > 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, bool no_quota_check); > + 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); > > diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c > index 60d3aa1ddb..7bc49ec97c 100644 > --- a/tools/xenstore/xenstored_domain.c > +++ b/tools/xenstore/xenstored_domain.c > @@ -523,7 +523,7 @@ static int domain_tree_remove_sub(const void *ctx, struct connection *conn, > node->perms.p[0].id = priv_domid; > node->acc.memory = 0; > domain_nbentry_inc(NULL, priv_domid); > - if (write_node_raw(NULL, &key, node, true)) { > + if (write_node_raw(NULL, &key, node, NODE_MODIFY, true)) { > /* That's unfortunate. We only can try to continue. */ > syslog(LOG_ERR, > "error when moving orphaned node %s to dom0\n", > diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c > index 334f1609f1..0655073de7 100644 > --- a/tools/xenstore/xenstored_transaction.c > +++ b/tools/xenstore/xenstored_transaction.c > @@ -290,7 +290,8 @@ int access_node(struct connection *conn, struct node *node, > i->check_gen = true; > if (node->generation != NO_GENERATION) { > set_tdb_key(i->trans_name, &local_key); > - ret = write_node_raw(conn, &local_key, node, true); > + ret = write_node_raw(conn, &local_key, node, > + NODE_CREATE, true); > if (ret) > goto err; > i->ta_node = true; > @@ -363,6 +364,7 @@ static int finalize_transaction(struct connection *conn, > TDB_DATA key, ta_key, data; > struct xs_tdb_record_hdr *hdr; > uint64_t gen; > + int flag; > > list_for_each_entry_safe(i, n, &trans->accessed, list) { > if (i->check_gen) { > @@ -405,8 +407,10 @@ static int finalize_transaction(struct connection *conn, > ta_key.dsize + data.dsize); > hdr = (void *)data.dptr; > hdr->generation = ++generation; > + flag = (i->generation == NO_GENERATION) > + ? NODE_CREATE : NODE_MODIFY; > *is_corrupt |= do_tdb_write(conn, &key, &data, > - NULL, true); > + NULL, flag, true); > talloc_free(data.dptr); > if (do_tdb_delete(conn, &ta_key, NULL)) > *is_corrupt = true; Cheers,
On 19.06.23 20:43, Julien Grall wrote: > Hi Juergen, > > On 30/05/2023 10:13, Juergen Gross wrote: >> Instead of using TDB_REPLACE for either creating or modifying a TDB >> entry, use either TDB_INSERT or TDB_MODIFY when calling tdb_store(). >> >> At higher function levels use the abstract flag values NODE_CREATE >> and NODE_MODIFY. >> >> This is for preparing to get rid of TDB. > > IIUC, you want to make the change so don't need to check if the node exists when > we are creating a new node. If so, isn't this change also beneficial for TDB? Hmm, maybe. > >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> tools/xenstore/xenstored_core.c | 29 ++++++++++++++------------ >> tools/xenstore/xenstored_core.h | 6 ++++-- >> tools/xenstore/xenstored_domain.c | 2 +- >> tools/xenstore/xenstored_transaction.c | 8 +++++-- >> 4 files changed, 27 insertions(+), 18 deletions(-) >> >> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c >> index 31a862b715..90c0bc7423 100644 >> --- a/tools/xenstore/xenstored_core.c >> +++ b/tools/xenstore/xenstored_core.c >> @@ -601,7 +601,7 @@ static unsigned int get_acc_domid(struct connection *conn, >> TDB_DATA *key, >> } >> int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data, >> - struct node_account_data *acc, bool no_quota_check) >> + struct node_account_data *acc, int flag, bool no_quota_check) > > 'int' is the wrong type for a flag. > > Is flag only meant to have two values? If yes, then it would be best to switch > to bool (with define if you still want explicit name). > > If not, then I think this should be an enum. > > Rather than bool, I would also be happy if you want enum with two values. The "int" is a leftover from a previous version where I used the TDB_* defines directly. I'll switch to an enum. > >> { >> struct xs_tdb_record_hdr *hdr = (void *)data->dptr; >> struct node_account_data old_acc = {}; >> @@ -635,7 +635,8 @@ int do_tdb_write(struct connection *conn, TDB_DATA *key, >> TDB_DATA *data, >> } >> /* TDB should set errno, but doesn't even set ecode AFAICT. */ >> - if (tdb_store(tdb_ctx, *key, *data, TDB_REPLACE) != 0) { >> + if (tdb_store(tdb_ctx, *key, *data, >> + (flag == NODE_CREATE) ? TDB_INSERT : TDB_MODIFY) != 0) { >> domain_memory_add_nochk(conn, new_domid, >> -data->dsize - key->dsize); >> /* Error path, so no quota check. */ >> @@ -774,7 +775,7 @@ static bool read_node_can_propagate_errno(void) >> } >> int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node, >> - bool no_quota_check) >> + int flag, bool no_quota_check) >> { >> TDB_DATA data; >> void *p; >> @@ -812,7 +813,7 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, >> struct node *node, >> p += node->datalen; >> memcpy(p, node->children, node->childlen); >> - if (do_tdb_write(conn, key, &data, &node->acc, no_quota_check)) >> + if (do_tdb_write(conn, key, &data, &node->acc, flag, no_quota_check)) >> return EIO; >> return 0; >> @@ -823,14 +824,14 @@ int write_node_raw(struct connection *conn, TDB_DATA >> *key, struct node *node, >> * node->key. This can later be used if the change needs to be reverted. >> */ >> static int write_node(struct connection *conn, struct node *node, >> - bool no_quota_check) >> + int flag, bool no_quota_check) >> { >> int ret; >> if (access_node(conn, node, NODE_ACCESS_WRITE, &node->key)) >> return errno; >> - ret = write_node_raw(conn, &node->key, node, no_quota_check); >> + ret = write_node_raw(conn, &node->key, node, flag, no_quota_check); >> if (ret && conn && conn->transaction) { >> /* >> * Reverting access_node() is hard, so just fail the >> @@ -1496,7 +1497,8 @@ static struct node *create_node(struct connection *conn, >> const void *ctx, >> goto err; >> } >> - ret = write_node(conn, i, false); >> + ret = write_node(conn, i, i->parent ? NODE_CREATE : NODE_MODIFY, >> + false); >> if (ret) >> goto err; >> @@ -1560,7 +1562,7 @@ static int do_write(const void *ctx, struct connection >> *conn, >> } else { >> node->data = in->buffer + offset; >> node->datalen = datalen; >> - if (write_node(conn, node, false)) >> + if (write_node(conn, node, NODE_MODIFY, false)) >> return errno; >> } >> @@ -1610,7 +1612,7 @@ static int remove_child_entry(struct connection *conn, >> struct node *node, >> memdel(node->children, offset, childlen + 1, node->childlen); >> node->childlen -= childlen + 1; >> - return write_node(conn, node, true); >> + return write_node(conn, node, NODE_MODIFY, true); >> } >> static int delete_child(struct connection *conn, >> @@ -1807,7 +1809,7 @@ static int do_set_perms(const void *ctx, struct >> connection *conn, >> if (domain_nbentry_inc(conn, get_node_owner(node))) >> return ENOMEM; >> - if (write_node(conn, node, false)) >> + if (write_node(conn, node, NODE_MODIFY, false)) >> return errno; >> fire_watches(conn, ctx, name, node, false, &old_perms); >> @@ -2321,7 +2323,7 @@ static void manual_node(const char *name, const char >> *child) >> if (child) >> node->childlen = strlen(child) + 1; >> - if (write_node(NULL, node, false)) >> + if (write_node(NULL, node, NODE_CREATE, false)) >> barf_perror("Could not create initial node %s", name); >> talloc_free(node); >> } >> @@ -3469,12 +3471,13 @@ void read_state_node(const void *ctx, const void *state) >> barf("allocation error restoring node"); >> set_tdb_key(parentname, &key); >> - if (write_node_raw(NULL, &key, parent, true)) >> + if (write_node_raw(NULL, &key, parent, NODE_MODIFY, true)) >> barf("write parent error restoring node"); >> } >> set_tdb_key(name, &key); >> - if (write_node_raw(NULL, &key, node, true)) >> + if (write_node_raw(NULL, &key, node, >> + strcmp(name, "/") ? NODE_CREATE : NODE_MODIFY, true)) > > I think a comment for the check would be useful. AFAUI, this is because "/" is > always created before restoring the state. Correct. I'll add a comment. Juergen
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index 31a862b715..90c0bc7423 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -601,7 +601,7 @@ static unsigned int get_acc_domid(struct connection *conn, TDB_DATA *key, } int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data, - struct node_account_data *acc, bool no_quota_check) + struct node_account_data *acc, int flag, bool no_quota_check) { struct xs_tdb_record_hdr *hdr = (void *)data->dptr; struct node_account_data old_acc = {}; @@ -635,7 +635,8 @@ int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data, } /* TDB should set errno, but doesn't even set ecode AFAICT. */ - if (tdb_store(tdb_ctx, *key, *data, TDB_REPLACE) != 0) { + if (tdb_store(tdb_ctx, *key, *data, + (flag == NODE_CREATE) ? TDB_INSERT : TDB_MODIFY) != 0) { domain_memory_add_nochk(conn, new_domid, -data->dsize - key->dsize); /* Error path, so no quota check. */ @@ -774,7 +775,7 @@ static bool read_node_can_propagate_errno(void) } int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node, - bool no_quota_check) + int flag, bool no_quota_check) { TDB_DATA data; void *p; @@ -812,7 +813,7 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node, p += node->datalen; memcpy(p, node->children, node->childlen); - if (do_tdb_write(conn, key, &data, &node->acc, no_quota_check)) + if (do_tdb_write(conn, key, &data, &node->acc, flag, no_quota_check)) return EIO; return 0; @@ -823,14 +824,14 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node, * node->key. This can later be used if the change needs to be reverted. */ static int write_node(struct connection *conn, struct node *node, - bool no_quota_check) + int flag, bool no_quota_check) { int ret; if (access_node(conn, node, NODE_ACCESS_WRITE, &node->key)) return errno; - ret = write_node_raw(conn, &node->key, node, no_quota_check); + ret = write_node_raw(conn, &node->key, node, flag, no_quota_check); if (ret && conn && conn->transaction) { /* * Reverting access_node() is hard, so just fail the @@ -1496,7 +1497,8 @@ static struct node *create_node(struct connection *conn, const void *ctx, goto err; } - ret = write_node(conn, i, false); + ret = write_node(conn, i, i->parent ? NODE_CREATE : NODE_MODIFY, + false); if (ret) goto err; @@ -1560,7 +1562,7 @@ static int do_write(const void *ctx, struct connection *conn, } else { node->data = in->buffer + offset; node->datalen = datalen; - if (write_node(conn, node, false)) + if (write_node(conn, node, NODE_MODIFY, false)) return errno; } @@ -1610,7 +1612,7 @@ static int remove_child_entry(struct connection *conn, struct node *node, memdel(node->children, offset, childlen + 1, node->childlen); node->childlen -= childlen + 1; - return write_node(conn, node, true); + return write_node(conn, node, NODE_MODIFY, true); } static int delete_child(struct connection *conn, @@ -1807,7 +1809,7 @@ static int do_set_perms(const void *ctx, struct connection *conn, if (domain_nbentry_inc(conn, get_node_owner(node))) return ENOMEM; - if (write_node(conn, node, false)) + if (write_node(conn, node, NODE_MODIFY, false)) return errno; fire_watches(conn, ctx, name, node, false, &old_perms); @@ -2321,7 +2323,7 @@ static void manual_node(const char *name, const char *child) if (child) node->childlen = strlen(child) + 1; - if (write_node(NULL, node, false)) + if (write_node(NULL, node, NODE_CREATE, false)) barf_perror("Could not create initial node %s", name); talloc_free(node); } @@ -3469,12 +3471,13 @@ void read_state_node(const void *ctx, const void *state) barf("allocation error restoring node"); set_tdb_key(parentname, &key); - if (write_node_raw(NULL, &key, parent, true)) + if (write_node_raw(NULL, &key, parent, NODE_MODIFY, true)) barf("write parent error restoring node"); } set_tdb_key(name, &key); - if (write_node_raw(NULL, &key, node, true)) + if (write_node_raw(NULL, &key, node, + strcmp(name, "/") ? NODE_CREATE : NODE_MODIFY, true)) barf("write node error restoring node"); if (domain_nbentry_inc(&conn, get_node_owner(node))) diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h index 84a611cbb5..9291efec17 100644 --- a/tools/xenstore/xenstored_core.h +++ b/tools/xenstore/xenstored_core.h @@ -238,7 +238,9 @@ static inline unsigned int get_node_owner(const struct node *node) /* Write a node to the tdb data base. */ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node, - bool no_quota_check); + int flag, bool no_quota_check); +#define NODE_CREATE 0 +#define NODE_MODIFY 1 /* Get a node from the tdb data base. */ struct node *read_node(struct connection *conn, const void *ctx, @@ -358,7 +360,7 @@ int remember_string(struct hashtable *hash, const char *str); 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, bool no_quota_check); + 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); diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c index 60d3aa1ddb..7bc49ec97c 100644 --- a/tools/xenstore/xenstored_domain.c +++ b/tools/xenstore/xenstored_domain.c @@ -523,7 +523,7 @@ static int domain_tree_remove_sub(const void *ctx, struct connection *conn, node->perms.p[0].id = priv_domid; node->acc.memory = 0; domain_nbentry_inc(NULL, priv_domid); - if (write_node_raw(NULL, &key, node, true)) { + if (write_node_raw(NULL, &key, node, NODE_MODIFY, true)) { /* That's unfortunate. We only can try to continue. */ syslog(LOG_ERR, "error when moving orphaned node %s to dom0\n", diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c index 334f1609f1..0655073de7 100644 --- a/tools/xenstore/xenstored_transaction.c +++ b/tools/xenstore/xenstored_transaction.c @@ -290,7 +290,8 @@ int access_node(struct connection *conn, struct node *node, i->check_gen = true; if (node->generation != NO_GENERATION) { set_tdb_key(i->trans_name, &local_key); - ret = write_node_raw(conn, &local_key, node, true); + ret = write_node_raw(conn, &local_key, node, + NODE_CREATE, true); if (ret) goto err; i->ta_node = true; @@ -363,6 +364,7 @@ static int finalize_transaction(struct connection *conn, TDB_DATA key, ta_key, data; struct xs_tdb_record_hdr *hdr; uint64_t gen; + int flag; list_for_each_entry_safe(i, n, &trans->accessed, list) { if (i->check_gen) { @@ -405,8 +407,10 @@ static int finalize_transaction(struct connection *conn, ta_key.dsize + data.dsize); hdr = (void *)data.dptr; hdr->generation = ++generation; + flag = (i->generation == NO_GENERATION) + ? NODE_CREATE : NODE_MODIFY; *is_corrupt |= do_tdb_write(conn, &key, &data, - NULL, true); + NULL, flag, true); talloc_free(data.dptr); if (do_tdb_delete(conn, &ta_key, NULL)) *is_corrupt = true;
Instead of using TDB_REPLACE for either creating or modifying a TDB entry, use either TDB_INSERT or TDB_MODIFY when calling tdb_store(). At higher function levels use the abstract flag values NODE_CREATE and NODE_MODIFY. This is for preparing to get rid of TDB. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenstore/xenstored_core.c | 29 ++++++++++++++------------ tools/xenstore/xenstored_core.h | 6 ++++-- tools/xenstore/xenstored_domain.c | 2 +- tools/xenstore/xenstored_transaction.c | 8 +++++-- 4 files changed, 27 insertions(+), 18 deletions(-)