[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()
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@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.
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
Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
|
