Re: [PATCH v2 18/18] tools/xenstore: add nocopy flag to node read functions

[Juergen Gross <jgross@xxxxxxxx>]

On 18.07.23 23:35, Julien Grall wrote:
> Hi Juergen,
>
> On 10/07/2023 07:59, Juergen Gross wrote:
> > Today when reading a node from the data base through read_node(), the
> > node data is copied in order to avoid modifying the data base when
> > preparing a node update, as otherwise an error might result in an
> > inconsistent state.
> >
> > There are, however, many cases where such a copy operation isn't
> > needed, as the node isn't modified.
> >
> > Add a "nocopy" flag to read_node() and get_node*() functions for making
> > those cases less memory consuming and more performant.
> Reducing memory consumption and improving performance is good. However you are 
> now relying on the caller to do the right thing when 'nocopy' is true. I believe 
> this is a disaster waiting to happen.
> So as it stands, I don't support this approach. The solution I have in mind 
> would require that 'struct node' is const for the 'nocopy' case. I agree this 
> means more work, but that's the price for reduce the the risk of corruption.
Fair enough.

I'll look into splitting read_node() into a direct variant returning a const
pointer and a variant copying the data. Same will be needed for get_node*().

> > Note that there is one modification of the node data left, which is not
> > problematic: domain_adjust_node_perms() might set the "ignore" flag of
> > a permission. This does no harm, as such an update of the permissions
> > doesn't need to be undone in case of a later processing error.
> Even if this is the "ignore" flag, this is definitely not an ideal situation. 
> And, AFAICT, this is not even document. I don't to be the reader trying to 
> figure out why read_node() and db_fetch() returns a slightly different node 
> content :).
So would you be fine with the addition of a comment explaining the situation?


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature