[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 17/25] tools/xenstore: rework struct xs_tdb_record_hdr
On 27.07.23 23:53, Julien Grall wrote:
Hi Juergen,
On 24/07/2023 12:02, Juergen Gross wrote:
Struct xs_tdb_record_hdr is used for nodes stored in the data base.
When working on a node, struct node is being used, which is including
the same information as struct xs_tdb_record_hdr, but in a different
format. Rework struct xs_tdb_record_hdr in order to prepare including
it in struct node.
Do the following modifications:
- move its definition to xenstored_core.h, as the reason to put it into
utils.h are no longer existing
- rename it to struct node_hdr, as the "tdb" in its name has only
historical reasons
- replace the empty permission array at the end with a comment about
the layout of data in the data base (concatenation of header,
permissions, node contents, and children list)
- use narrower types for num_perms and datalen, as those are naturally
limited to XENSTORE_PAYLOAD_MAX (childlen is different here, as it is
in theory basically unlimited)
The assumption is XENSTORE_PAYLOAD_MAX will never change and/or always apply for
all the connection. I am aware of at least one downstream use where this is not
the case.
I am happy to use narrower types, but I would at least like some checks in
Xenstore to ensure the limits are not reached.
I will add a BUILD_BUG_ON().
Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
---
V2:
- new patch
---
tools/xenstore/utils.h | 9 -------
tools/xenstore/xenstored_core.c | 35 +++++++++++++++-----------
tools/xenstore/xenstored_core.h | 20 ++++++++++++++-
tools/xenstore/xenstored_transaction.c | 6 ++---
4 files changed, 42 insertions(+), 28 deletions(-)
diff --git a/tools/xenstore/utils.h b/tools/xenstore/utils.h
index 028ecb9d7a..405d662ea2 100644
--- a/tools/xenstore/utils.h
+++ b/tools/xenstore/utils.h
@@ -9,15 +9,6 @@
#include "xenstore_lib.h"
-/* Header of the node record in tdb. */
-struct xs_tdb_record_hdr {
- uint64_t generation;
- uint32_t num_perms;
- uint32_t datalen;
- uint32_t childlen;
- struct xs_permissions perms[0];
-};
-
/* Is A == B ? */
#define streq(a,b) (strcmp((a),(b)) == 0)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 1f5f118f1c..86b7c9bf36 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -555,9 +555,9 @@ static void initialize_fds(int *p_sock_pollfd_idx, int
*ptimeout)
}
}
-const struct xs_tdb_record_hdr *db_fetch(const char *db_name, size_t *size)
+const struct node_hdr *db_fetch(const char *db_name, size_t *size)
{
- struct xs_tdb_record_hdr *hdr;
+ struct node_hdr *hdr;
hdr = hashtable_search(nodes, db_name);
if (!hdr) {
@@ -565,7 +565,7 @@ const struct xs_tdb_record_hdr *db_fetch(const char
*db_name, size_t *size)
return NULL;
}
- *size = sizeof(*hdr) + hdr->num_perms * sizeof(hdr->perms[0]) +
+ *size = sizeof(*hdr) + hdr->num_perms * sizeof(struct xs_permissions) +
hdr->datalen + hdr->childlen;
trace_tdb("read %s size %zu\n", db_name, *size + strlen(db_name));
@@ -573,10 +573,15 @@ const struct xs_tdb_record_hdr *db_fetch(const char
*db_name, size_t *size)
return hdr;
}
+static struct xs_permissions *perms_from_node_hdr(const struct node_hdr *hdr)
I don't much like the casting-away the const here. Looking at the code, most of
the users seems to not modify the value. So how about return const here and
open-coding or casting-away the const in the caller (with a proper explanation)?
I'll look into that.
+{
+ return (struct xs_permissions *)(hdr + 1);
+}
+
static void get_acc_data(const char *name, struct node_account_data *acc)
{
size_t size;
- const struct xs_tdb_record_hdr *hdr;
+ const struct node_hdr *hdr;
if (acc->memory < 0) {
hdr = db_fetch(name, &size);
@@ -585,7 +590,7 @@ static void get_acc_data(const char *name, struct
node_account_data *acc)
acc->memory = 0;
} else {
acc->memory = size;
- acc->domid = hdr->perms[0].id;
+ acc->domid = perms_from_node_hdr(hdr)->id;
}
}
}
@@ -606,7 +611,7 @@ int db_write(struct connection *conn, const char *db_name,
const void *data,
size_t size, struct node_account_data *acc,
enum write_node_mode mode, bool no_quota_check)
{
- const struct xs_tdb_record_hdr *hdr = data;
+ const struct node_hdr *hdr = data;
struct node_account_data old_acc = {};
unsigned int old_domid, new_domid;
size_t name_len = strlen(db_name);
@@ -620,7 +625,7 @@ int db_write(struct connection *conn, const char *db_name,
const void *data,
get_acc_data(db_name, &old_acc);
old_domid = get_acc_domid(conn, db_name, old_acc.domid);
- new_domid = get_acc_domid(conn, db_name, hdr->perms[0].id);
+ new_domid = get_acc_domid(conn, db_name, perms_from_node_hdr(hdr)->id);
/*
* Don't check for ENOENT, as we want to be able to switch orphaned
@@ -661,7 +666,7 @@ int db_write(struct connection *conn, const char *db_name,
const void *data,
if (acc) {
/* Don't use new_domid, as it might be a transaction node. */
- acc->domid = hdr->perms[0].id;
+ acc->domid = perms_from_node_hdr(hdr)->id;
acc->memory = size;
}
@@ -699,7 +704,7 @@ struct node *read_node(struct connection *conn, const void
*ctx,
const char *name)
{
size_t size;
- const struct xs_tdb_record_hdr *hdr;
+ const struct node_hdr *hdr;
struct node *node;
const char *db_name;
int err;
@@ -733,12 +738,12 @@ struct node *read_node(struct connection *conn, const
void *ctx,
node->perms.num = hdr->num_perms;
node->datalen = hdr->datalen;
node->childlen = hdr->childlen;
- node->acc.domid = hdr->perms[0].id;
+ node->acc.domid = perms_from_node_hdr(hdr)->id;
node->acc.memory = size;
/* Copy node data to new memory area, starting with permissions. */
size -= sizeof(*hdr);
- node->perms.p = talloc_memdup(node, hdr->perms, size);
+ node->perms.p = talloc_memdup(node, perms_from_node_hdr(hdr), size);
if (node->perms.p == NULL) {
errno = ENOMEM;
goto error;
@@ -785,7 +790,7 @@ int write_node_raw(struct connection *conn, const char
*db_name,
void *data;
size_t size;
void *p;
- struct xs_tdb_record_hdr *hdr;
+ struct node_hdr *hdr;
if (domain_adjust_node_perms(node))
return errno;
@@ -812,9 +817,9 @@ int write_node_raw(struct connection *conn, const char
*db_name,
hdr->datalen = node->datalen;
hdr->childlen = node->childlen;
- memcpy(hdr->perms, node->perms.p,
- node->perms.num * sizeof(*node->perms.p));
- p = hdr->perms + node->perms.num;
+ p = perms_from_node_hdr(hdr);
+ memcpy(p, node->perms.p, node->perms.num * sizeof(*node->perms.p));
+ p += node->perms.num * sizeof(*node->perms.p);
memcpy(p, node->data, node->datalen);
p += node->datalen;
memcpy(p, node->children, node->childlen);
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 6d1578ce97..c965709090 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -168,6 +168,24 @@ struct connection
};
extern struct list_head connections;
+/*
+ * Header of the node record in the data base.
+ * In the data base the memory of the node is a single memory chunk with the
+ * following format:
+ * struct {
+ * node_hdr hdr;
+ * struct xs_permissions perms[hdr.num_perms];
+ * char data[hdr.datalen];
+ * char children[hdr.childlen];
+ * };
+ */
+struct node_hdr {
+ uint64_t generation;
+ uint16_t num_perms;
OOI, do you have a use case where a node would be shared with more than 255
domains?
No, but limiting it wouldn't give any real advantage.
+ uint16_t datalen;
+ uint32_t childlen;
+};
+
struct node_perms {
unsigned int num;
struct xs_permissions *p;
@@ -362,7 +380,7 @@ extern xengnttab_handle **xgt_handle;
int remember_string(struct hashtable *hash, const char *str);
/* Data base access functions. */
-const struct xs_tdb_record_hdr *db_fetch(const char *db_name, size_t *size);
+const struct node_hdr *db_fetch(const char *db_name, size_t *size);
int db_write(struct connection *conn, const char *db_name, const void *data,
size_t size, struct node_account_data *acc,
enum write_node_mode mode, bool no_quota_check);
diff --git a/tools/xenstore/xenstored_transaction.c
b/tools/xenstore/xenstored_transaction.c
index a90283dcc5..9ca73b9874 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -357,7 +357,7 @@ static int finalize_transaction(struct connection *conn,
{
struct accessed_node *i, *n;
size_t size;
- const struct xs_tdb_record_hdr *hdr;
+ const struct node_hdr *hdr;
uint64_t gen;
list_for_each_entry_safe(i, n, &trans->accessed, list) {
@@ -394,12 +394,12 @@ static int finalize_transaction(struct connection *conn,
* generation count.
*/
enum write_node_mode mode;
- struct xs_tdb_record_hdr *own;
+ struct node_hdr *own;
talloc_increase_ref_count(hdr);
db_delete(conn, i->trans_name, NULL);
- own = (struct xs_tdb_record_hdr *)hdr;
+ own = (struct node_hdr *)hdr;
own->generation = ++generation;
mode = (i->generation == NO_GENERATION)
? NODE_CREATE : NODE_MODIFY;
Juergen
Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
|
