Re: [PATCH V1 03/26] migration: SAVEVM_FOREACH

[Peter Xu <peterx@redhat.com>]

On Mon, May 13, 2024 at 03:27:30PM -0400, Steven Sistare wrote:
> On 5/6/2024 7:17 PM, Fabiano Rosas wrote:
> > Steve Sistare <steven.sistare@oracle.com> writes:
> > 
> > > Define an abstraction SAVEVM_FOREACH to loop over all savevm state
> > > handlers, and replace QTAILQ_FOREACH.  Define variants for ALL so
> > > we can loop over all handlers vs a subset of handlers in a subsequent
> > > patch, but at this time there is no distinction between the two.
> > > No functional change.
> > > 
> > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> > > ---
> > >   migration/savevm.c | 55 +++++++++++++++++++++++++++++++-----------------------
> > >   1 file changed, 32 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > index 4509482..6829ba3 100644
> > > --- a/migration/savevm.c
> > > +++ b/migration/savevm.c
> > > @@ -237,6 +237,15 @@ static SaveState savevm_state = {
> > >       .global_section_id = 0,
> > >   };
> > > +#define SAVEVM_FOREACH(se, entry)                                    \
> > > +    QTAILQ_FOREACH(se, &savevm_state.handlers, entry)                \
> > > +
> > > +#define SAVEVM_FOREACH_ALL(se, entry)                                \
> > > +    QTAILQ_FOREACH(se, &savevm_state.handlers, entry)
> > 
> > This feels worse than SAVEVM_FOREACH_NOT_PRECREATED. We'll have to keep
> > coming back to the definition to figure out which FOREACH is the real
> > deal.
> 
> I take your point, but the majority of the loops do not care about precreated
> objects, so it seems backwards to make them more verbose with
> SAVEVM_FOREACH_NOT_PRECREATE.  I can go either way, but we need
> Peter's opinion also.

I don't have a strong opinion yet on the name, I think it'll be clearer
indeed when the _ALL() (or whatever other name) is used only when with the
real users.

OTOH, besides the name (which is much more trivial..) the precreated idea
in general is a bit scary to me.. if that was for a "workaround" to some
new ordering issue due to newly added dependencies on exec() support.
Maybe I'll understand better when I get to know better on the whole design.

-- 
Peter Xu