Re: [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.05.2020 16:04, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 19 May 2020 14:04
>>
>> On 14.05.2020 12:44, Paul Durrant wrote:
>>> --- /dev/null
>>> +++ b/xen/include/xen/save.h
>>> @@ -0,0 +1,165 @@
>>> +/*
>>> + * save.h: support routines for save/restore
>>> + *
>>> + * Copyright Amazon.com Inc. or its affiliates.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms and conditions of the GNU General Public License,
>>> + * version 2, as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License 
>>> for
>>> + * more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License along 
>>> with
>>> + * this program; If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#ifndef XEN_SAVE_H
>>> +#define XEN_SAVE_H
>>> +
>>> +#include <xen/init.h>
>>> +#include <xen/sched.h>
>>> +#include <xen/types.h>
>>> +
>>> +#include <public/save.h>
>>> +
>>> +struct domain_context;
>>> +
>>> +int domain_save_begin(struct domain_context *c, unsigned int typecode,
>>> +                      const char *name, unsigned int instance);
>>> +
>>> +#define DOMAIN_SAVE_BEGIN(_x, _c, _instance) \
>>> +    domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_instance))
>>
>> As per prior conversation I would have expected no leading underscores
>> here anymore, and no parenthesization of what is still _c. More of
>> these further down.
> 
> What's wrong with leading underscores in macro arguments? They can't
> pollute any namespace.

They can still hide file scope variables legitimately named
this way (which may get accessed by a macro without being a
macro argument). The wording of the standard is quite clear:
"All identifiers that begin with an underscore are always
reserved for use as identifiers with file scope in both the
ordinary and tag name spaces."

> Also, I prefer to keep the parentheses for arguments.

More code churn then once we hopefully standardize of the less
obfuscating variant without.

>>> +static inline int domain_load_entry(struct domain_context *c,
>>> +                                    unsigned int typecode, const char 
>>> *name,
>>> +                                    unsigned int *instance, void *dst,
>>> +                                    size_t len)
>>> +{
>>> +    int rc;
>>> +
>>> +    rc = domain_load_begin(c, typecode, name, instance);
>>
>> For some reason I've spotted this only here: Why is instance a pointer
>> parameter of the function, when typecode is a value? Both live next to
>> one another in struct domain_save_descriptor, and hence instance could
>> be retrieved at the same time as typecode.
>>
> 
> Because the typecode is known a priori. It has to be known for the
> correct handler to be invoked. It is only provided here for
> verification purposes. I could have provided the instance as an
> argument to the load handler but I prefer making the interactions
> for save and load more symmetric.

Hmm, I don't see any symmetry violation in the alternative model,
but anyway.

>>> +/*
>>> + * Register save and restore handlers. Save handlers will be invoked
>>> + * in order of DOMAIN_SAVE_CODE().
>>> + */
>>> +#define DOMAIN_REGISTER_SAVE_RESTORE(_x, _save, _load)            \
>>> +    static int __init __domain_register_##_x##_save_restore(void) \
>>> +    {                                                             \
>>> +        domain_register_save_type(                                \
>>> +            DOMAIN_SAVE_CODE(_x),                                 \
>>> +            #_x,                                                  \
>>> +            &(_save),                                             \
>>> +            &(_load));                                            \
>>> +                                                                  \
>>> +        return 0;                                                 \
>>> +    }                                                             \
>>> +    __initcall(__domain_register_##_x##_save_restore);
>>
>> I'm puzzled by part of the comment: Invoking by save code looks
>> reasonable for the saving side (albeit END doesn't match this rule
>> afaics), but is this going to be good enough for the consuming side?
> 
> No, this only relates to the save side which is why the comment
> says 'Save handlers'. I do note that it would be more consistent
> to use 'load' rather than 'restore' here though.
> 
>> There may be dependencies between types, and with fixed ordering
>> there may be no way to insert a depended upon type ahead of an
>> already defined one (at least as long as the codes are meant to be
>> stable).
>>
> 
> The ordering of load handlers is determined by the stream. I'll
> add a sentence saying that.

I.e. the consumer of the "get" interface (and producer of the stream)
is supposed to take apart the output it gets, bring records into
suitable order (which implies it knows of all the records, and which
hence means this code may need updating in cases where I'd expect
only the hypervisor needs), and only then issue to the stream?

Jan