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

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(-)

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.

On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala 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);
> @@ -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;
>  
> diff --git a/migration/exec.h b/migration/exec.h
> index b210ffde7a..736cd71028 100644
> --- a/migration/exec.h
> +++ b/migration/exec.h
> @@ -19,6 +19,10 @@
>  
>  #ifndef QEMU_MIGRATION_EXEC_H
>  #define QEMU_MIGRATION_EXEC_H
> +
> +#ifdef WIN32
> +const char *exec_get_cmd_path(void);
> +#endif
>  void exec_start_incoming_migration(const char *host_port, Error **errp);
>  
>  void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
> diff --git a/migration/migration.c b/migration/migration.c
> index a7e4e286aa..61f52d2f90 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -421,7 +421,11 @@ static bool migrate_uri_parse(const char *uri,
>  
>      if (strstart(uri, "exec:", NULL)) {
>          addrs->transport = MIGRATE_TRANSPORT_EXEC;
> +#ifdef WIN32
> +        QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
> +#else
>          QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> +#endif

This windows portability code should have been in the previous patch
I think.

>          QAPI_LIST_APPEND(tail, g_strdup("-c"));
>          QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
>      } else if (strstart(uri, "rdma:", NULL) &&
> @@ -450,8 +454,10 @@ static bool migrate_uri_parse(const char *uri,
>  
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
> +    Error *local_err = NULL;
>      const char *p = NULL;
>      MigrateAddress *channel = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr;
>  
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
> @@ -463,23 +469,32 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>          goto out;
>      }
>  
> +    saddr = &channel->u.socket;

Accessing u.socket before checkout transport == SOCKET is bad
practice, even though this is technically safe.

>      qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> -    if (strstart(uri, "tcp:", &p) ||
> -        strstart(uri, "unix:", NULL) ||
> -        strstart(uri, "vsock:", NULL)) {
> -        socket_start_incoming_migration(p ? p : uri, errp);
> +    if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {

THis should have

    SocketAddress *saddr = &channe->u.socket

so that 'saddr' is limited in scope to where we've validated
transport == SOCKET

> +        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> +            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
> +            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> +            socket_start_incoming_migration(saddr, &local_err);
> +        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
> +            fd_start_incoming_migration(saddr->u.fd.str, &local_err);
> +        }
>  #ifdef CONFIG_RDMA
>      } else if (strstart(uri, "rdma:", &p)) {
>          rdma_start_incoming_migration(p, errp);
>  #endif
>      } else if (strstart(uri, "exec:", &p)) {
>          exec_start_incoming_migration(p, errp);
> -    } else if (strstart(uri, "fd:", &p)) {
> -        fd_start_incoming_migration(p, errp);
>      } else {
>          error_setg(errp, "unknown migration protocol: %s", uri);
>      }
>  
> +    if (local_err) {
> +        qapi_free_SocketAddress(saddr);
> +        error_propagate(errp, local_err);
> +        return;

THis leaks 'channel', and free's 'saddr' which actually  belongs
to channel.

With my comments on the previous patch suggesting g_autoptr for
'channel', we don't need any free calls for 'saddr' or 'channel'.

> +    }
> +
>  out:
>      qapi_free_MigrateAddress(channel);
>  }
> @@ -1688,6 +1703,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      MigrationState *s = migrate_get_current();
>      const char *p = NULL;
>      MigrateAddress *channel = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr;
>  
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
> @@ -1711,18 +1727,21 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          }
>      }
>  
> -    if (strstart(uri, "tcp:", &p) ||
> -        strstart(uri, "unix:", NULL) ||
> -        strstart(uri, "vsock:", NULL)) {
> -        socket_start_outgoing_migration(s, p ? p : uri, &local_err);
> +    saddr = &channel->u.socket;

Again, put this *after*  checking transport == SOCKET

> +    if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
> +        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> +            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
> +            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> +            socket_start_outgoing_migration(s, saddr, &local_err);
> +        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
> +            fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
> +        }
>  #ifdef CONFIG_RDMA
>      } else if (strstart(uri, "rdma:", &p)) {
>          rdma_start_outgoing_migration(s, p, &local_err);
>  #endif
>      } else if (strstart(uri, "exec:", &p)) {
>          exec_start_outgoing_migration(s, p, &local_err);
> -    } else if (strstart(uri, "fd:", &p)) {
> -        fd_start_outgoing_migration(s, p, &local_err);
>      } else {
>          if (!(has_resume && resume)) {
>              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> @@ -1739,6 +1758,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          if (!(has_resume && resume)) {
>              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>          }
> +        qapi_free_SocketAddress(saddr);

This saddr pointer belongs to 'channel' which must be freed.

>          migrate_fd_error(s, local_err);
>          error_propagate(errp, local_err);
>          return;
> diff --git a/migration/socket.c b/migration/socket.c
> index 1b6f5baefb..8e7430b266 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -108,10 +108,9 @@ out:
>      object_unref(OBJECT(sioc));
>  }
>  
> -static void
> -socket_start_outgoing_migration_internal(MigrationState *s,
> -                                         SocketAddress *saddr,
> -                                         Error **errp)
> +void socket_start_outgoing_migration(MigrationState *s,
> +                                     SocketAddress *saddr,
> +                                     Error **errp)
>  {
>      QIOChannelSocket *sioc = qio_channel_socket_new();
>      struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
> @@ -135,18 +134,6 @@ socket_start_outgoing_migration_internal(MigrationState *s,
>                                       NULL);
>  }
>  
> -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);
> -}
> -
>  static void socket_accept_incoming_migration(QIONetListener *listener,
>                                               QIOChannelSocket *cioc,
>                                               gpointer opaque)
> @@ -172,9 +159,8 @@ socket_incoming_migration_end(void *opaque)
>      object_unref(OBJECT(listener));
>  }
>  
> -static void
> -socket_start_incoming_migration_internal(SocketAddress *saddr,
> -                                         Error **errp)
> +void socket_start_incoming_migration(SocketAddress *saddr,
> +                                     Error **errp)
>  {
>      QIONetListener *listener = qio_net_listener_new();
>      MigrationIncomingState *mis = migration_incoming_get_current();
> @@ -213,13 +199,3 @@ socket_start_incoming_migration_internal(SocketAddress *saddr,
>      }
>  }
>  
> -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);
> -}
> diff --git a/migration/socket.h b/migration/socket.h
> index dc54df4e6c..5e4c33b8ea 100644
> --- a/migration/socket.h
> +++ b/migration/socket.h
> @@ -19,13 +19,14 @@
>  
>  #include "io/channel.h"
>  #include "io/task.h"
> +#include "qemu/sockets.h"
>  
>  void socket_send_channel_create(QIOTaskFunc f, void *data);
>  QIOChannel *socket_send_channel_create_sync(Error **errp);
>  int socket_send_channel_destroy(QIOChannel *send);
>  
> -void socket_start_incoming_migration(const char *str, Error **errp);
> +void socket_start_incoming_migration(SocketAddress *saddr, Error **errp);
>  
> -void socket_start_outgoing_migration(MigrationState *s, const char *str,
> -                                     Error **errp);
> +void socket_start_outgoing_migration(MigrationState *s,
> +                                     SocketAddress *saddr, Error **errp);
>  #endif
> -- 
> 2.22.3
> 

With regards,
Daniel

On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
> On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala 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)
Even this, I will shift it to the 2nd patch, where I need to move 
exec_get_cmd_path() out accross other file (migration.c).
>>   {
>>       g_autofree char *detected_path = g_new(char, MAX_PATH);
>> @@ -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;
Sure, Juan. Will change this in the 2nd patch itself instead of here. I 
am not very convinved why should we have a different patch all together 
for this, because we are just using this code outside this file in my 
opinion? But if you still think so, I can make a different patch for that.
>>   
>> diff --git a/migration/exec.h b/migration/exec.h
>> index b210ffde7a..736cd71028 100644
>> --- a/migration/exec.h
>> +++ b/migration/exec.h
>> @@ -19,6 +19,10 @@
>>   
>>   #ifndef QEMU_MIGRATION_EXEC_H
>>   #define QEMU_MIGRATION_EXEC_H
>> +
>> +#ifdef WIN32
>> +const char *exec_get_cmd_path(void);
>> +#endif
>>   void exec_start_incoming_migration(const char *host_port, Error **errp);
>>   
>>   void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
>> diff --git a/migration/migration.c b/migration/migration.c
>> index a7e4e286aa..61f52d2f90 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -421,7 +421,11 @@ static bool migrate_uri_parse(const char *uri,
>>   
>>       if (strstart(uri, "exec:", NULL)) {
>>           addrs->transport = MIGRATE_TRANSPORT_EXEC;
>> +#ifdef WIN32
>> +        QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
>> +#else
>>           QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
>> +#endif
> This windows portability code should have been in the previous patch
> I think.
Ack, yes, it could have also been added in the 2nd patch. Will 
accomodate windows portability change there itself.
>>           QAPI_LIST_APPEND(tail, g_strdup("-c"));
>>           QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
>>       } else if (strstart(uri, "rdma:", NULL) &&
>> @@ -450,8 +454,10 @@ static bool migrate_uri_parse(const char *uri,
>>   
>>   static void qemu_start_incoming_migration(const char *uri, Error **errp)
>>   {
>> +    Error *local_err = NULL;
>>       const char *p = NULL;
>>       MigrateAddress *channel = g_new0(MigrateAddress, 1);
>> +    SocketAddress *saddr;
>>   
>>       /* URI is not suitable for migration? */
>>       if (!migration_channels_and_uri_compatible(uri, errp)) {
>> @@ -463,23 +469,32 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>>           goto out;
>>       }
>>   
>> +    saddr = &channel->u.socket;
> Accessing u.socket before checkout transport == SOCKET is bad
> practice, even though this is technically safe.

Ok, Ack. I understand your point, before defining whether transport 
could be a socket or not, it is not a good way representing and defining 
it earlier.

Thanks for pointing it out Daniel. I will change this in all the places, 
where I have accessed it before confirming if the transport == socket.

>>       qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>> -    if (strstart(uri, "tcp:", &p) ||
>> -        strstart(uri, "unix:", NULL) ||
>> -        strstart(uri, "vsock:", NULL)) {
>> -        socket_start_incoming_migration(p ? p : uri, errp);
>> +    if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
> THis should have
>
>      SocketAddress *saddr = &channe->u.socket
>
> so that 'saddr' is limited in scope to where we've validated
> transport == SOCKET
Yes, Ack. Makes sense.
>> +        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>> +            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>> +            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>> +            socket_start_incoming_migration(saddr, &local_err);
>> +        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
>> +            fd_start_incoming_migration(saddr->u.fd.str, &local_err);
>> +        }
>>   #ifdef CONFIG_RDMA
>>       } else if (strstart(uri, "rdma:", &p)) {
>>           rdma_start_incoming_migration(p, errp);
>>   #endif
>>       } else if (strstart(uri, "exec:", &p)) {
>>           exec_start_incoming_migration(p, errp);
>> -    } else if (strstart(uri, "fd:", &p)) {
>> -        fd_start_incoming_migration(p, errp);
>>       } else {
>>           error_setg(errp, "unknown migration protocol: %s", uri);
>>       }
>>   
>> +    if (local_err) {
>> +        qapi_free_SocketAddress(saddr);
>> +        error_propagate(errp, local_err);
>> +        return;
Juan, I get your point. But I think, we won't be needing local_err at 
all, if I use g_autoptr for 'channel' and 'saddr' is a part of 
'channel'. Let me have a v2 patchset and if it is still not convinving, 
we can have a discussion on this.
> THis leaks 'channel', and free's 'saddr' which actually  belongs
> to channel.
>
> With my comments on the previous patch suggesting g_autoptr for
> 'channel', we don't need any free calls for 'saddr' or 'channel'.

Right. With g_autoptr used for freeing 'channel' in last patch, we wont 
have to worry about freeing 'saddr' at all. Thanks Daniel

if (local_err) {
     qapi_free_SocketAddress(saddr);
     error_propagate(errp, local_err);
     return;
}
And after changing the position for assigning 'saddr' and using 
g_autoptr for 'channel' I believe we can get rid of 'local_error' 
variable too and replace it with 'errp'. Please suggest if I am missing 
something here. TIA!

>> +    }
>> +
>>   out:
>>       qapi_free_MigrateAddress(channel);
>>   }
>> @@ -1688,6 +1703,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>       MigrationState *s = migrate_get_current();
>>       const char *p = NULL;
>>       MigrateAddress *channel = g_new0(MigrateAddress, 1);
>> +    SocketAddress *saddr;
>>   
>>       /* URI is not suitable for migration? */
>>       if (!migration_channels_and_uri_compatible(uri, errp)) {
>> @@ -1711,18 +1727,21 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>           }
>>       }
>>   
>> -    if (strstart(uri, "tcp:", &p) ||
>> -        strstart(uri, "unix:", NULL) ||
>> -        strstart(uri, "vsock:", NULL)) {
>> -        socket_start_outgoing_migration(s, p ? p : uri, &local_err);
>> +    saddr = &channel->u.socket;
> Again, put this *after*  checking transport == SOCKET
Ack.
>> +    if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
>> +        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>> +            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>> +            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>> +            socket_start_outgoing_migration(s, saddr, &local_err);
>> +        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
>> +            fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
>> +        }
>>   #ifdef CONFIG_RDMA
>>       } else if (strstart(uri, "rdma:", &p)) {
>>           rdma_start_outgoing_migration(s, p, &local_err);
>>   #endif
>>       } else if (strstart(uri, "exec:", &p)) {
>>           exec_start_outgoing_migration(s, p, &local_err);
>> -    } else if (strstart(uri, "fd:", &p)) {
>> -        fd_start_outgoing_migration(s, p, &local_err);
>>       } else {
>>           if (!(has_resume && resume)) {
>>               yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>> @@ -1739,6 +1758,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>           if (!(has_resume && resume)) {
>>               yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>>           }
>> +        qapi_free_SocketAddress(saddr);
> This saddr pointer belongs to 'channel' which must be freed.
Again here, with your comment in last patch to use g_autoptr, we need 
not to explicitly free 'channel' or 'saddr' right.
>>           migrate_fd_error(s, local_err);
>>           error_propagate(errp, local_err);
>>           return;
>> diff --git a/migration/socket.c b/migration/socket.c
>> index 1b6f5baefb..8e7430b266 100644
>> --- a/migration/socket.c
>> +++ b/migration/socket.c
>> @@ -108,10 +108,9 @@ out:
>>       object_unref(OBJECT(sioc));
>>   }
>>   
>> -static void
>> -socket_start_outgoing_migration_internal(MigrationState *s,
>> -                                         SocketAddress *saddr,
>> -                                         Error **errp)
>> +void socket_start_outgoing_migration(MigrationState *s,
>> +                                     SocketAddress *saddr,
>> +                                     Error **errp)
>>   {
>>       QIOChannelSocket *sioc = qio_channel_socket_new();
>>       struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
>> @@ -135,18 +134,6 @@ socket_start_outgoing_migration_internal(MigrationState *s,
>>                                        NULL);
>>   }
>>   
>> -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);
>> -}
>> -
Actually Juan. I don't need this function at all, because parsing of uri 
into socketAddress using socket_parse is already done. So there is no 
use of having this function in the first place, so I decided to delete 
this fucntion all together. Same with incoming function.
>>   static void socket_accept_incoming_migration(QIONetListener *listener,
>>                                                QIOChannelSocket *cioc,
>>                                                gpointer opaque)
>> @@ -172,9 +159,8 @@ socket_incoming_migration_end(void *opaque)
>>       object_unref(OBJECT(listener));
>>   }
>>   
>> -static void
>> -socket_start_incoming_migration_internal(SocketAddress *saddr,
>> -                                         Error **errp)
>> +void socket_start_incoming_migration(SocketAddress *saddr,
>> +                                     Error **errp)
>>   {
>>       QIONetListener *listener = qio_net_listener_new();
>>       MigrationIncomingState *mis = migration_incoming_get_current();
>> @@ -213,13 +199,3 @@ socket_start_incoming_migration_internal(SocketAddress *saddr,
>>       }
>>   }
>>   
>> -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);
>> -}
>> diff --git a/migration/socket.h b/migration/socket.h
>> index dc54df4e6c..5e4c33b8ea 100644
>> --- a/migration/socket.h
>> +++ b/migration/socket.h
>> @@ -19,13 +19,14 @@
>>   
>>   #include "io/channel.h"
>>   #include "io/task.h"
>> +#include "qemu/sockets.h"
>>   
>>   void socket_send_channel_create(QIOTaskFunc f, void *data);
>>   QIOChannel *socket_send_channel_create_sync(Error **errp);
>>   int socket_send_channel_destroy(QIOChannel *send);
>>   
>> -void socket_start_incoming_migration(const char *str, Error **errp);
>> +void socket_start_incoming_migration(SocketAddress *saddr, Error **errp);
>>   
>> -void socket_start_outgoing_migration(MigrationState *s, const char *str,
>> -                                     Error **errp);
>> +void socket_start_outgoing_migration(MigrationState *s,
>> +                                     SocketAddress *saddr, Error **errp);
>>   #endif
>> -- 
>> 2.22.3
>>
> With regards,
> Daniel

Het Gala <het.gala@nutanix.com> wrote:
> On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
>> On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala 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)
> Even this, I will shift it to the 2nd patch, where I need to move
> exec_get_cmd_path() out accross other file (migration.c).

great.

>>>   {
>>>       g_autofree char *detected_path = g_new(char, MAX_PATH);
>>> @@ -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;
> Sure, Juan. Will change this in the 2nd patch itself instead of
> here. I am not very convinved why should we have a different patch all
> together for this, because we are just using this code outside this
> file in my opinion? But if you still think so, I can make a different
> patch for that.

It is up to you.


> Juan, I get your point. But I think, we won't be needing local_err at
> all, if I use g_autoptr for 'channel' and 'saddr' is a part of
> 'channel'. Let me have a v2 patchset and if it is still not
> convinving, we can have a discussion on this.
>> THis leaks 'channel', and free's 'saddr' which actually  belongs
>> to channel.
>>
>> With my comments on the previous patch suggesting g_autoptr for
>> 'channel', we don't need any free calls for 'saddr' or 'channel'.
>
> Right. With g_autoptr used for freeing 'channel' in last patch, we
> wont have to worry about freeing 'saddr' at all. Thanks Daniel
>
> if (local_err) {
>     qapi_free_SocketAddress(saddr);
>     error_propagate(errp, local_err);
>     return;
> }
> And after changing the position for assigning 'saddr' and using
> g_autoptr for 'channel' I believe we can get rid of 'local_error'
> variable too and replace it with 'errp'. Please suggest if I am
> missing something here. TIA!

great.  That is much better.

>>>   -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);
>>> -}
>>> -
> Actually Juan. I don't need this function at all, because parsing of
> uri into socketAddress using socket_parse is already done. So there is
> no use of having this function in the first place, so I decided to
> delete this fucntion all together. Same with incoming function.

What I mean is that this code was already there.  And it was doing it
wrong.  The parts that I corrected you were using this pattern, chcking
that err was NULL, intsead of cheking that saddr is NULL.

Later, Juan.

On 15/05/23 8:16 pm, Juan Quintela wrote:
> Het Gala <het.gala@nutanix.com> wrote:
>> On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
>>> On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala 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)
>> Even this, I will shift it to the 2nd patch, where I need to move
>> exec_get_cmd_path() out accross other file (migration.c).
> great.
>
>>>>    {
>>>>        g_autofree char *detected_path = g_new(char, MAX_PATH);
>>>> @@ -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;
>> Sure, Juan. Will change this in the 2nd patch itself instead of
>> here. I am not very convinved why should we have a different patch all
>> together for this, because we are just using this code outside this
>> file in my opinion? But if you still think so, I can make a different
>> patch for that.
> It is up to you.

For now, I will keep it with 2nd patch. If any other maintainer also 
thinks that it should be a separate patch all together of windows 
support for exec, I will make a new patch for that. But thankyou for 
your suggestion 😁

>> Juan, I get your point. But I think, we won't be needing local_err at
>> all, if I use g_autoptr for 'channel' and 'saddr' is a part of
>> 'channel'. Let me have a v2 patchset and if it is still not
>> convinving, we can have a discussion on this.
>>> THis leaks 'channel', and free's 'saddr' which actually  belongs
>>> to channel.
>>>
>>> With my comments on the previous patch suggesting g_autoptr for
>>> 'channel', we don't need any free calls for 'saddr' or 'channel'.
>> Right. With g_autoptr used for freeing 'channel' in last patch, we
>> wont have to worry about freeing 'saddr' at all. Thanks Daniel
>>
>> if (local_err) {
>>      qapi_free_SocketAddress(saddr);
>>      error_propagate(errp, local_err);
>>      return;
>> }
>> And after changing the position for assigning 'saddr' and using
>> g_autoptr for 'channel' I believe we can get rid of 'local_error'
>> variable too and replace it with 'errp'. Please suggest if I am
>> missing something here. TIA!
> great.  That is much better.
Ack.
>>>>    -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);
>>>> -}
>>>> -
>> Actually Juan. I don't need this function at all, because parsing of
>> uri into socketAddress using socket_parse is already done. So there is
>> no use of having this function in the first place, so I decided to
>> delete this fucntion all together. Same with incoming function.
> What I mean is that this code was already there.  And it was doing it
> wrong.  The parts that I corrected you were using this pattern, chcking
> that err was NULL, intsead of cheking that saddr is NULL.
Yes, I get your point. But that function is useless if socket_parse() 
function is not needed. So have omitted the function all together as 
socket parsing is already done in earlier patches.
> Later, Juan.
Regards,
Het Gala

On 15/05/23 8:46 pm, Het Gala wrote:
>
> On 15/05/23 8:16 pm, Juan Quintela wrote:
>> Het Gala <het.gala@nutanix.com> wrote:
>>> On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
>>>> On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala 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)
Maintainers please advice. If I want to see thatthe build is proper, how 
to enable WIN32 support on a CentOS guest operating system (what all 
dependencies to install, what to configure for a build which supports 
WIN32) ? Any pointers ?
>>> Even this, I will shift it to the 2nd patch, where I need to move
>>> exec_get_cmd_path() out accross other file (migration.c).
>> great.
>>
>>>>>    {
>>>>>        g_autofree char *detected_path = g_new(char, MAX_PATH);
>>>>> @@ -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;
>>> Sure, Juan. Will change this in the 2nd patch itself instead of
>>> here. I am not very convinved why should we have a different patch all
>>> together for this, because we are just using this code outside this
>>> file in my opinion? But if you still think so, I can make a different
>>> patch for that.
>> It is up to you.
>
> For now, I will keep it with 2nd patch. If any other maintainer also 
> thinks that it should be a separate patch all together of windows 
> support for exec, I will make a new patch for that. But thankyou for 
> your suggestion 😁
>
>>> Juan, I get your point. But I think, we won't be needing local_err at
>>> all, if I use g_autoptr for 'channel' and 'saddr' is a part of
>>> 'channel'. Let me have a v2 patchset and if it is still not
>>> convinving, we can have a discussion on this.
>>>> THis leaks 'channel', and free's 'saddr' which actually  belongs
>>>> to channel.
>>>>
>>>> With my comments on the previous patch suggesting g_autoptr for
>>>> 'channel', we don't need any free calls for 'saddr' or 'channel'.
>>> Right. With g_autoptr used for freeing 'channel' in last patch, we
>>> wont have to worry about freeing 'saddr' at all. Thanks Daniel
>>>
>>> if (local_err) {
>>>      qapi_free_SocketAddress(saddr);
>>>      error_propagate(errp, local_err);
>>>      return;
>>> }
>>> And after changing the position for assigning 'saddr' and using
>>> g_autoptr for 'channel' I believe we can get rid of 'local_error'
>>> variable too and replace it with 'errp'. Please suggest if I am
>>> missing something here. TIA!
>> great.  That is much better.
> Ack.
>>>>>    -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);
>>>>> -}
>>>>> -
>>> Actually Juan. I don't need this function at all, because parsing of
>>> uri into socketAddress using socket_parse is already done. So there is
>>> no use of having this function in the first place, so I decided to
>>> delete this fucntion all together. Same with incoming function.
>> What I mean is that this code was already there.  And it was doing it
>> wrong.  The parts that I corrected you were using this pattern, chcking
>> that err was NULL, intsead of cheking that saddr is NULL.
> Yes, I get your point. But that function is useless if socket_parse() 
> function is not needed. So have omitted the function all together as 
> socket parsing is already done in earlier patches.
>> Later, Juan.
> Regards,
> Het Gala

On Mon, May 15, 2023 at 09:58:37PM +0530, Het Gala wrote:
> 
> On 15/05/23 8:46 pm, Het Gala wrote:
> > 
> > On 15/05/23 8:16 pm, Juan Quintela wrote:
> > > Het Gala <het.gala@nutanix.com> wrote:
> > > > On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
> > > > > On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala 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)
> Maintainers please advice. If I want to see thatthe build is proper, how to
> enable WIN32 support on a CentOS guest operating system (what all
> dependencies to install, what to configure for a build which supports WIN32)
> ? Any pointers ?

QEMU does Windows build testing using Fedora, which ships mingw
packages. If you only have centos, then use docker/podman to
get a Fedora environment in a container locally.

Alternatively push to gitlab and run a CI pipeline (see docs/devel/ci.rst
for more info)

> > > > Even this, I will shift it to the 2nd patch, where I need to move
> > > > exec_get_cmd_path() out accross other file (migration.c).
> > > great.
> > > 
> > > > > >    {
> > > > > >        g_autofree char *detected_path = g_new(char, MAX_PATH);
> > > > > > @@ -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;
> > > > Sure, Juan. Will change this in the 2nd patch itself instead of
> > > > here. I am not very convinved why should we have a different patch all
> > > > together for this, because we are just using this code outside this
> > > > file in my opinion? But if you still think so, I can make a different
> > > > patch for that.
> > > It is up to you.
> > 
> > For now, I will keep it with 2nd patch. If any other maintainer also
> > thinks that it should be a separate patch all together of windows
> > support for exec, I will make a new patch for that. But thankyou for
> > your suggestion 😁
> > 
> > > > Juan, I get your point. But I think, we won't be needing local_err at
> > > > all, if I use g_autoptr for 'channel' and 'saddr' is a part of
> > > > 'channel'. Let me have a v2 patchset and if it is still not
> > > > convinving, we can have a discussion on this.
> > > > > THis leaks 'channel', and free's 'saddr' which actually  belongs
> > > > > to channel.
> > > > > 
> > > > > With my comments on the previous patch suggesting g_autoptr for
> > > > > 'channel', we don't need any free calls for 'saddr' or 'channel'.
> > > > Right. With g_autoptr used for freeing 'channel' in last patch, we
> > > > wont have to worry about freeing 'saddr' at all. Thanks Daniel
> > > > 
> > > > if (local_err) {
> > > >      qapi_free_SocketAddress(saddr);
> > > >      error_propagate(errp, local_err);
> > > >      return;
> > > > }
> > > > And after changing the position for assigning 'saddr' and using
> > > > g_autoptr for 'channel' I believe we can get rid of 'local_error'
> > > > variable too and replace it with 'errp'. Please suggest if I am
> > > > missing something here. TIA!
> > > great.  That is much better.
> > Ack.
> > > > > >    -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);
> > > > > > -}
> > > > > > -
> > > > Actually Juan. I don't need this function at all, because parsing of
> > > > uri into socketAddress using socket_parse is already done. So there is
> > > > no use of having this function in the first place, so I decided to
> > > > delete this fucntion all together. Same with incoming function.
> > > What I mean is that this code was already there.  And it was doing it
> > > wrong.  The parts that I corrected you were using this pattern, chcking
> > > that err was NULL, intsead of cheking that saddr is NULL.
> > Yes, I get your point. But that function is useless if socket_parse()
> > function is not needed. So have omitted the function all together as
> > socket parsing is already done in earlier patches.
> > > Later, Juan.
> > Regards,
> > Het Gala
> 

With regards,
Daniel