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

[Julien Grall <julien@xxxxxxx>]

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.
> 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 :).
Cheers,

--
Julien Grall