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

[Juan Quintela]

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.

> 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.

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.

Later, Juan.