Re: [PATCH v2 4/4] migration: Allow postcopy_ram_supported_by_host() to report err

[Peter Xu <peterx@redhat.com>]

On Wed, Apr 19, 2023 at 09:51:24PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Instead of print it to STDERR, bring the error upwards so that it can be
> > reported via QMP responses.
> >
> > E.g.:
> >
> > { "execute": "migrate-set-capabilities" ,
> >   "arguments": { "capabilities":
> >   [ { "capability": "postcopy-ram", "state": true } ] } }
> >
> > { "error":
> >   { "class": "GenericError",
> >     "desc": "Postcopy is not supported: Host backend files need to be TMPFS
> >     or HUGETLBFS only" } }
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c    |  9 +++---
> >  migration/postcopy-ram.c | 61 ++++++++++++++++++++++------------------
> >  migration/postcopy-ram.h |  3 +-
> >  migration/savevm.c       |  3 +-
> >  4 files changed, 42 insertions(+), 34 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index bda4789193..ac15fa6092 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1313,6 +1313,7 @@ static bool migrate_caps_check(bool *cap_list,
> >      MigrationCapabilityStatusList *cap;
> >      bool old_postcopy_cap;
> >      MigrationIncomingState *mis = migration_incoming_get_current();
> > +    Error *local_err = NULL;
> 
> This variable can be declared in the only block that uses it.

Sure, though I just remembered I can use ERRP_GUARD(), hence I'll go with
that.

> 
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index bbb8af61ae..0713ddeeef 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -282,11 +282,14 @@ static bool request_ufd_features(int ufd, uint64_t features)
> >      return true;
> >  }
> >  
> > -static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
> > +static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis,
> > +                                Error **errp)
> >  {
> >      uint64_t asked_features = 0;
> >      static uint64_t supported_features;
> >  
> > +    assert(errp);
> > +
> 
> Is this right?  My impression was that you have to live with errp being NULL.

Right it knows, I just wanted to make sure error msg got captured, but we
don't really need to worrry here, all callers are in this case I believe.

Let me also switch to ERRP_GUARD() just to still avoid error_abort being
passed in, so we can still abort with a full message.

> 
> error_setg() knows how to handle it being NULL:
> 
> error_setg() -> error_setg_internal() -> error_setv()
> 
> static void error_setv(Error **errp,
>                        const char *src, int line, const char *func,
>                        ErrorClass err_class, const char *fmt, va_list ap,
>                        const char *suffix)
> {
>     ....
>     if (errp == NULL) {
>         return;
>     }
> 
> 
> > -static int test_ramblock_postcopiable(RAMBlock *rb)
> > +static int test_ramblock_postcopiable(RAMBlock *rb, Error **errp)
> 
> In patch 3 you do this other change:
> 
> -static int test_ramblock_postcopiable(RAMBlock *rb, void *opaque)
> +static int test_ramblock_postcopiable(RAMBlock *rb)
> 
> Can you do with a single change?
> 
> The idea of the patch is right.

I saw that patch 3 already merged.  I'll just spin this one, thanks!

-- 
Peter Xu