On Wed, Apr 19, 2023 at 09:51:24PM +0200, Juan Quintela wrote:
> Peter Xu <pet...@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 <pet...@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
