| Message ID | 20230710065947.4201-15-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: drop TDB | expand |
Hi Juergen, On 10/07/2023 07:59, Juergen Gross wrote: > Today the node data is copied in db_fetch() on each data base read in > order to avoid accidental data base modifications when working on a > node. > > read_node() is the only caller of db_fetch() which isn't freeing the > returned data area immediately after using it. The other callers don't > modify the returned data, so they don't need the data to be copied. This reads as the return value of db_fetch() should be const. This will at least make more obvious to the caller that the value is not supposed to be modified. > > Move copying of the data into read_node(), resulting in a speedup of > the other callers due to no memory allocation and no copying being > needed anymore. > > As db_fetch() can't return any error other than ENOENT now, error > handling for the callers can be simplified. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > V2: > - new patch > --- > tools/xenstore/xenstored_core.c | 41 ++++++++++---------------- > tools/xenstore/xenstored_transaction.c | 3 -- > 2 files changed, 16 insertions(+), 28 deletions(-) > > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c > index 11da470b20..d5c9054fe9 100644 > --- a/tools/xenstore/xenstored_core.c > +++ b/tools/xenstore/xenstored_core.c > @@ -557,8 +557,7 @@ static void initialize_fds(int *p_sock_pollfd_idx, int *ptimeout) > > struct xs_tdb_record_hdr *db_fetch(const char *db_name, size_t *size) > { > - const struct xs_tdb_record_hdr *hdr; > - struct xs_tdb_record_hdr *p; > + struct xs_tdb_record_hdr *hdr; > > hdr = hashtable_search(nodes, db_name); > if (!hdr) { > @@ -569,18 +568,9 @@ struct xs_tdb_record_hdr *db_fetch(const char *db_name, size_t *size) > *size = sizeof(*hdr) + hdr->num_perms * sizeof(hdr->perms[0]) + > hdr->datalen + hdr->childlen; > > - p = talloc_size(NULL, *size); > - if (!p) { > - errno = ENOMEM; > - return NULL; > - } > - > trace_tdb("read %s size %zu\n", db_name, *size + strlen(db_name)); > > - /* Return a copy, avoiding a potential modification in the DB. */ > - memcpy(p, hdr, *size); > - > - return p; > + return hdr; > } > > static void get_acc_data(const char *name, struct node_account_data *acc) > @@ -597,7 +587,6 @@ static void get_acc_data(const char *name, struct node_account_data *acc) > acc->memory = size; > acc->domid = hdr->perms[0].id; > } > - talloc_free(hdr); > } > } > > @@ -731,30 +720,32 @@ struct node *read_node(struct connection *conn, const void *ctx, > hdr = db_fetch(db_name, &size); > > if (hdr == NULL) { > - if (errno == ENOENT) { > - node->generation = NO_GENERATION; > - err = access_node(conn, node, NODE_ACCESS_READ, NULL); > - errno = err ? : ENOENT; > - } else { > - log("DB error on read: %s", strerror(errno)); > - errno = EIO; > - } > + node->generation = NO_GENERATION; > + err = access_node(conn, node, NODE_ACCESS_READ, NULL); > + errno = err ? : ENOENT; > goto error; > } > > node->parent = NULL; > - talloc_steal(node, hdr); > > /* Datalen, childlen, number of permissions */ > node->generation = hdr->generation; > node->perms.num = hdr->num_perms; > node->datalen = hdr->datalen; > node->childlen = hdr->childlen; > + node->acc.domid = hdr->perms[0].id; > + node->acc.memory = size; > + > + /* Copy node data to new memory area, starting with permissions. */ > + size -= sizeof(*hdr); > + node->perms.p = talloc_size(node, size); > + if (node->perms.p == NULL) { > + errno = ENOMEM; > + goto error; > + } > + memcpy(node->perms.p, hdr->perms, size); > > /* Permissions are struct xs_permissions. */ Is this comment still relevant? > - node->perms.p = hdr->perms; > - node->acc.domid = get_node_owner(node); > - node->acc.memory = size; > if (domain_adjust_node_perms(node)) > goto error; > > diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c > index 5d7da82aad..e3e05a1d84 100644 > --- a/tools/xenstore/xenstored_transaction.c > +++ b/tools/xenstore/xenstored_transaction.c > @@ -365,13 +365,10 @@ static int finalize_transaction(struct connection *conn, > if (i->check_gen) { > hdr = db_fetch(i->node, &size); > if (!hdr) { > - if (errno != ENOENT) > - return errno; > gen = NO_GENERATION; > } else { > gen = hdr->generation; > } > - talloc_free(hdr); > if (i->generation != gen) > return EAGAIN; > } Cheers,
On 18.07.23 23:10, Julien Grall wrote: > Hi Juergen, > > On 10/07/2023 07:59, Juergen Gross wrote: >> Today the node data is copied in db_fetch() on each data base read in >> order to avoid accidental data base modifications when working on a >> node. >> >> read_node() is the only caller of db_fetch() which isn't freeing the >> returned data area immediately after using it. The other callers don't >> modify the returned data, so they don't need the data to be copied. > > This reads as the return value of db_fetch() should be const. This will at least > make more obvious to the caller that the value is not supposed to be modified. This will add some more code churn. In the end I expect this to be much more sane, though (e.g. talloc_free() taking a const pointer). I'll look into that. > >> >> Move copying of the data into read_node(), resulting in a speedup of >> the other callers due to no memory allocation and no copying being >> needed anymore. >> >> As db_fetch() can't return any error other than ENOENT now, error >> handling for the callers can be simplified. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> V2: >> - new patch >> --- >> tools/xenstore/xenstored_core.c | 41 ++++++++++---------------- >> tools/xenstore/xenstored_transaction.c | 3 -- >> 2 files changed, 16 insertions(+), 28 deletions(-) >> >> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c >> index 11da470b20..d5c9054fe9 100644 >> --- a/tools/xenstore/xenstored_core.c >> +++ b/tools/xenstore/xenstored_core.c >> @@ -557,8 +557,7 @@ static void initialize_fds(int *p_sock_pollfd_idx, int >> *ptimeout) >> struct xs_tdb_record_hdr *db_fetch(const char *db_name, size_t *size) >> { >> - const struct xs_tdb_record_hdr *hdr; >> - struct xs_tdb_record_hdr *p; >> + struct xs_tdb_record_hdr *hdr; >> hdr = hashtable_search(nodes, db_name); >> if (!hdr) { >> @@ -569,18 +568,9 @@ struct xs_tdb_record_hdr *db_fetch(const char *db_name, >> size_t *size) >> *size = sizeof(*hdr) + hdr->num_perms * sizeof(hdr->perms[0]) + >> hdr->datalen + hdr->childlen; >> - p = talloc_size(NULL, *size); >> - if (!p) { >> - errno = ENOMEM; >> - return NULL; >> - } >> - >> trace_tdb("read %s size %zu\n", db_name, *size + strlen(db_name)); >> - /* Return a copy, avoiding a potential modification in the DB. */ >> - memcpy(p, hdr, *size); >> - >> - return p; >> + return hdr; >> } >> static void get_acc_data(const char *name, struct node_account_data *acc) >> @@ -597,7 +587,6 @@ static void get_acc_data(const char *name, struct >> node_account_data *acc) >> acc->memory = size; >> acc->domid = hdr->perms[0].id; >> } >> - talloc_free(hdr); >> } >> } >> @@ -731,30 +720,32 @@ struct node *read_node(struct connection *conn, const >> void *ctx, >> hdr = db_fetch(db_name, &size); >> if (hdr == NULL) { >> - if (errno == ENOENT) { >> - node->generation = NO_GENERATION; >> - err = access_node(conn, node, NODE_ACCESS_READ, NULL); >> - errno = err ? : ENOENT; >> - } else { >> - log("DB error on read: %s", strerror(errno)); >> - errno = EIO; >> - } >> + node->generation = NO_GENERATION; >> + err = access_node(conn, node, NODE_ACCESS_READ, NULL); >> + errno = err ? : ENOENT; >> goto error; >> } >> node->parent = NULL; >> - talloc_steal(node, hdr); >> /* Datalen, childlen, number of permissions */ >> node->generation = hdr->generation; >> node->perms.num = hdr->num_perms; >> node->datalen = hdr->datalen; >> node->childlen = hdr->childlen; >> + node->acc.domid = hdr->perms[0].id; >> + node->acc.memory = size; >> + >> + /* Copy node data to new memory area, starting with permissions. */ >> + size -= sizeof(*hdr); >> + node->perms.p = talloc_size(node, size); >> + if (node->perms.p == NULL) { >> + errno = ENOMEM; >> + goto error; >> + } >> + memcpy(node->perms.p, hdr->perms, size); >> /* Permissions are struct xs_permissions. */ > > Is this comment still relevant? I was on the edge, too. With you asking that question I'm happy to remove the comment. Juergen
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index 11da470b20..d5c9054fe9 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -557,8 +557,7 @@ static void initialize_fds(int *p_sock_pollfd_idx, int *ptimeout) struct xs_tdb_record_hdr *db_fetch(const char *db_name, size_t *size) { - const struct xs_tdb_record_hdr *hdr; - struct xs_tdb_record_hdr *p; + struct xs_tdb_record_hdr *hdr; hdr = hashtable_search(nodes, db_name); if (!hdr) { @@ -569,18 +568,9 @@ struct xs_tdb_record_hdr *db_fetch(const char *db_name, size_t *size) *size = sizeof(*hdr) + hdr->num_perms * sizeof(hdr->perms[0]) + hdr->datalen + hdr->childlen; - p = talloc_size(NULL, *size); - if (!p) { - errno = ENOMEM; - return NULL; - } - trace_tdb("read %s size %zu\n", db_name, *size + strlen(db_name)); - /* Return a copy, avoiding a potential modification in the DB. */ - memcpy(p, hdr, *size); - - return p; + return hdr; } static void get_acc_data(const char *name, struct node_account_data *acc) @@ -597,7 +587,6 @@ static void get_acc_data(const char *name, struct node_account_data *acc) acc->memory = size; acc->domid = hdr->perms[0].id; } - talloc_free(hdr); } } @@ -731,30 +720,32 @@ struct node *read_node(struct connection *conn, const void *ctx, hdr = db_fetch(db_name, &size); if (hdr == NULL) { - if (errno == ENOENT) { - node->generation = NO_GENERATION; - err = access_node(conn, node, NODE_ACCESS_READ, NULL); - errno = err ? : ENOENT; - } else { - log("DB error on read: %s", strerror(errno)); - errno = EIO; - } + node->generation = NO_GENERATION; + err = access_node(conn, node, NODE_ACCESS_READ, NULL); + errno = err ? : ENOENT; goto error; } node->parent = NULL; - talloc_steal(node, hdr); /* Datalen, childlen, number of permissions */ node->generation = hdr->generation; node->perms.num = hdr->num_perms; node->datalen = hdr->datalen; node->childlen = hdr->childlen; + node->acc.domid = hdr->perms[0].id; + node->acc.memory = size; + + /* Copy node data to new memory area, starting with permissions. */ + size -= sizeof(*hdr); + node->perms.p = talloc_size(node, size); + if (node->perms.p == NULL) { + errno = ENOMEM; + goto error; + } + memcpy(node->perms.p, hdr->perms, size); /* Permissions are struct xs_permissions. */ - node->perms.p = hdr->perms; - node->acc.domid = get_node_owner(node); - node->acc.memory = size; if (domain_adjust_node_perms(node)) goto error; diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c index 5d7da82aad..e3e05a1d84 100644 --- a/tools/xenstore/xenstored_transaction.c +++ b/tools/xenstore/xenstored_transaction.c @@ -365,13 +365,10 @@ static int finalize_transaction(struct connection *conn, if (i->check_gen) { hdr = db_fetch(i->node, &size); if (!hdr) { - if (errno != ENOENT) - return errno; gen = NO_GENERATION; } else { gen = hdr->generation; } - talloc_free(hdr); if (i->generation != gen) return EAGAIN; }
Today the node data is copied in db_fetch() on each data base read in order to avoid accidental data base modifications when working on a node. read_node() is the only caller of db_fetch() which isn't freeing the returned data area immediately after using it. The other callers don't modify the returned data, so they don't need the data to be copied. Move copying of the data into read_node(), resulting in a speedup of the other callers due to no memory allocation and no copying being needed anymore. As db_fetch() can't return any error other than ENOENT now, error handling for the callers can be simplified. Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - new patch --- tools/xenstore/xenstored_core.c | 41 ++++++++++---------------- tools/xenstore/xenstored_transaction.c | 3 -- 2 files changed, 16 insertions(+), 28 deletions(-)