Re: [PATCH v4 3/8] migration: converts socket backend to accept MigrateAddress struct

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
> Socket transport backend for 'migrate'/'migrate-incoming' QAPIs accept
> new wire protocol of MigrateAddress struct.
>
> It is achived by parsing 'uri' string and storing migration parameters
> required for socket connection into well defined SocketAddress struct.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  migration/exec.c      |  4 ++--
>  migration/exec.h      |  4 ++++
>  migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
>  migration/socket.c    | 34 +++++----------------------------
>  migration/socket.h    |  7 ++++---
>  5 files changed, 47 insertions(+), 46 deletions(-)
>
> diff --git a/migration/exec.c b/migration/exec.c
> index 2bf882bbe1..c4a3293246 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -27,7 +27,6 @@
>  #include "qemu/cutils.h"
>  
>  #ifdef WIN32
> -const char *exec_get_cmd_path(void);
>  const char *exec_get_cmd_path(void)
>  {
>      g_autofree char *detected_path = g_new(char, MAX_PATH);

Make this and related chunks it its own patch.


> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
>  }
>  #endif
>  
> -void exec_start_outgoing_migration(MigrationState *s, const char *command, 
> Error **errp)
> +void exec_start_outgoing_migration(MigrationState *s, const char *command,
> +                                   Error **errp)
>  {
>      QIOChannel *ioc;
>  

Drop this bit.  If you want to cleanup longer that 80 chars lines, do it
in a sperate patch.


> +    if (local_err) {
> +        qapi_free_SocketAddress(saddr);
> +        error_propagate(errp, local_err);
> +        return;
> +    }

Aha, I see what you want to do here, but I 

>From include/qapi/error.h

 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.  Note that
 *   we still have many functions returning void.  We recommend
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.
 *


I think that what you have to do here is drop local_err and just change

socket_start_incoming_migration() and fd_start_incoming_migration() to
return a bool.

> -void socket_start_outgoing_migration(MigrationState *s,
> -                                     const char *str,
> -                                     Error **errp)
> -{
> -    Error *err = NULL;
> -    SocketAddress *saddr = socket_parse(str, &err);
> -    if (!err) {
> -        socket_start_outgoing_migration_internal(s, saddr, &err);
> -    }
> -    error_propagate(errp, err);
> -}

And here we are.  This function got it wrong, and you copied from here.
My understanding is that this function should have been written as:

void socket_start_outgoing_migration(MigrationState *s,
                                     const char *str,
                                     Error **errp)
{
    SocketAddress *saddr = socket_parse(str, &err);
    if (!saddr) {
        socket_start_outgoing_migration_internal(s, saddr, &err);
    }
}


> -void socket_start_incoming_migration(const char *str, Error **errp)
> -{
> -    Error *err = NULL;
> -    SocketAddress *saddr = socket_parse(str, &err);
> -    if (!err) {
> -        socket_start_incoming_migration_internal(saddr, &err);
> -    }
> -    qapi_free_SocketAddress(saddr);
> -    error_propagate(errp, err);
> -}

Exactly the same here.

Not your fault, but can you do the changes, please?

Later, Juan.