> -----Original Message-----
> From: Jan Beulich <[email protected]>
> Sent: 19 May 2020 15:24
> To: [email protected]
> Cc: [email protected]; 'Paul Durrant' <[email protected]>;
> 'Andrew Cooper'
> <[email protected]>; 'George Dunlap' <[email protected]>; 'Ian
> Jackson'
> <[email protected]>; 'Julien Grall' <[email protected]>; 'Stefano
> Stabellini'
> <[email protected]>; 'Wei Liu' <[email protected]>; 'Volodymyr Babchuk'
> <[email protected]>;
> 'Roger Pau Monné' <[email protected]>
> Subject: Re: [PATCH v3 1/5] xen/common: introduce a new framework for
> save/restore of 'domain' context
>
> On 19.05.2020 16:04, Paul Durrant wrote:
> >> From: Jan Beulich <[email protected]>
> >> 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."
>
Ok.
> > Also, I prefer to keep the parentheses for arguments.
>
> More code churn then once we hopefully standardize of the less
> obfuscating variant without.
>
If we standardize that way, so be it.
> >>> +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?
The intention is that the stream is always in a suitable order so the load side
does not have to do any re-ordering.
Paul
