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

[Juergen Gross <jgross@xxxxxxxx>]

On 27.07.23 23:33, Julien Grall wrote:
> 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?
Oh yes, I think so.

> >       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.
Thanks. In fact this is a fix resulting from letting db_fetch() return a const
pointer. So the const attribute already paid off.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature