[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 01/11] tools/xenstore: explicitly specify create or modify for tdb_store()
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@xxxxxxxx>
---
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,
--
Julien Grall
|
