[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 16/25] tools/xenstore: move copying of node data out of db_fetch()



Hi Juergen,

On 24/07/2023 12:02, 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.

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.

This allows to let db_fetch() return a pointer to const data.

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@xxxxxxxx>
---
V2:
- new patch
V3:
- modify return type of db_fetch() to return a pointer to const
   (Julien Grall)
- drop stale comment (Julien Grall)
- fix transaction handling
---
  tools/xenstore/xenstored_core.c        | 45 +++++++++++---------------
  tools/xenstore/xenstored_core.h        |  2 +-
  tools/xenstore/xenstored_transaction.c | 23 +++++++++----
  3 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 9f88914149..1f5f118f1c 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -555,10 +555,9 @@ 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 *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;

Can't 'hdr' stay const?

hdr = hashtable_search(nodes, db_name);
        if (!hdr) {

[...]

@@ -388,14 +385,26 @@ static int finalize_transaction(struct connection *conn,
                if (i->ta_node) {
                        hdr = db_fetch(i->trans_name, &size);
                        if (hdr) {
+                               /*
+                                * Delete transaction entry and write it as
+                                * no-TA entry. As we only hold a reference
+                                * to the data, increment its ref count, then
+                                * delete it from the DB. Now we own it and can
+                                * drop the const attribute for changing the

It is not great, but I understand this is another necessary evil. So I am ok with this cast-away const. It is also well documented.

+                                * generation count.
+                                */
                                enum write_node_mode mode;
+                               struct xs_tdb_record_hdr *own;
- hdr->generation = ++generation;
+                               talloc_increase_ref_count(hdr);
+                               db_delete(conn, i->trans_name, NULL);
+
+                               own = (struct xs_tdb_record_hdr *)hdr;
+                               own->generation = ++generation;
                                mode = (i->generation == NO_GENERATION)
                                       ? NODE_CREATE : NODE_MODIFY;
-                               *is_corrupt |= db_write(conn, i->node, hdr,
+                               *is_corrupt |= db_write(conn, i->node, own,
                                                        size, NULL, mode, true);
-                               db_delete(conn, i->trans_name, NULL);
                        } else {
                                *is_corrupt = true;
                        }

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.