Re: [PATCH v4 2/8] migration: Converts uri parameter into 'MigrateAddress' struct

[Het Gala <het.gala@nutanix.com>]

Just so that, there is a wider attention, I will try to address and 
discuss the comments from Daniel and Juan both here, as many of them 
seems to be overlapping. I hope that is fine with the maintainers.

On 15/05/23 3:42 pm, Daniel P. Berrangé wrote:
> On Fri, May 12, 2023 at 02:32:34PM +0000, Het Gala wrote:
>> This patch introduces code that can parse 'uri' string parameter and
>> spit out 'MigrateAddress' struct. All the required migration parameters
>> are stored in the struct.
>>
>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
>>   migration/migration.c | 63 +++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 61 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 0ee07802a5..a7e4e286aa 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -64,6 +64,7 @@
>>   #include "yank_functions.h"
>>   #include "sysemu/qtest.h"
>>   #include "options.h"
>> +#include "qemu/sockets.h"
>>   
>>   static NotifierList migration_state_notifiers =
>>       NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
>> @@ -408,13 +409,58 @@ void migrate_add_address(SocketAddress *address)
>>                         QAPI_CLONE(SocketAddress, address));
>>   }
>>   
>> +static bool migrate_uri_parse(const char *uri,
>> +                              MigrateAddress **channel,
>> +                              Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
>> +    SocketAddress *saddr;
>> +    InetSocketAddress *isock = &addrs->u.rdma;
>> +    strList **tail = &addrs->u.exec.args;
>> +
>> +    if (strstart(uri, "exec:", NULL)) {
>> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
>> +        QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
>> +        QAPI_LIST_APPEND(tail, g_strdup("-c"));
>> +        QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
>> +    } else if (strstart(uri, "rdma:", NULL) &&
>> +               !inet_parse(isock, uri + strlen("rdma:"), errp)) {
>> +        addrs->transport = MIGRATE_TRANSPORT_RDMA;
> I would have this as
>
>      } else if (strstart(uri, "rdma:", NULL)) {
>          if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
>              addrs->transport = MIGRATE_TRANSPORT_RDMA;
> 	}
>
> as IMHO it is bad practice to have control pass to the next
> else if clause when inet_parse() fails, as we know this is
> only an RDMA addr
Ack. I will change in the next patch.
> Also you need to use '&local_err' not 'errp' in the inet_parse
> call, otherwise the later code block for cleanup won't run.

Yes, thanks for pointing it out Daniel. Will modify that.

Also, Juan is of the opinion that we could omit 'local_error' variable 
and try to address and free the memory there itself. For ex:

if (saddr == NULL) {
     qapi_free_MigrateAddress(addrs);
     return false;
}

Or, Daniel, can I also define here the variables like you suggested down 
in the patch ? or is it used in some special case or I am missing 
something ?

g_autoptr(MigrateAddress) addrs = g_new0(MigrateAddress, 1);

So we would not have to worry to free MigrateAddress struct.

>> +    } else if (strstart(uri, "tcp:", NULL) ||
>> +                strstart(uri, "unix:", NULL) ||
>> +                strstart(uri, "vsock:", NULL) ||
>> +                strstart(uri, "fd:", NULL)) {
>> +        addrs->transport = MIGRATE_TRANSPORT_SOCKET;
>> +        saddr = socket_parse(uri, &local_err);
>> +        addrs->u.socket = *saddr;
> Protect with
>
>     if (saddr != NULL) {
>         addrs->u.socket = *saddr;
>     }
>
>> +    }
>> +
>> +    if (local_err) {
>> +        qapi_free_MigrateAddress(addrs);
>> +        qapi_free_SocketAddress(saddr);
>> +        qapi_free_InetSocketAddress(isock);
>> +        error_propagate(errp, local_err);
>> +        return false;
>> +    }
>> +
>> +    *channel = addrs;
>> +    return true;
>> +}
>> +
>>   static void qemu_start_incoming_migration(const char *uri, Error **errp)
>>   {
>>       const char *p = NULL;
>> +    MigrateAddress *channel = g_new0(MigrateAddress, 1);
> Avoid the later 'out:' cleanup block by using:
>
>    g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
Ack. I think this also solves the doubt raised by Juan "I wish, I really 
wish, that there was a way to free things on error". Am I right ?
>>   
>>       /* URI is not suitable for migration? */
>>       if (!migration_channels_and_uri_compatible(uri, errp)) {
>> -        return;
>> +        goto out;
>> +    }
>> +
>> +    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
>> +        error_setg(errp, "Error parsing uri");
> This error_setg() is overwriting the error migrate_uri_parse already
> set, so just drop it.

Okay, I got the point. I thought error_setg() would add error statement 
on top of errp, but I was incorrect.

Juan also made the same point I believe. I will remove error_setg() in 
the next version of patchset series.

>> +        goto out;
>>       }
>>   
>>       qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>> @@ -433,6 +479,9 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>>       } else {
>>           error_setg(errp, "unknown migration protocol: %s", uri);
>>       }
>> +
>> +out:
>> +    qapi_free_MigrateAddress(channel);
>>   }
>>   
>>   static void process_incoming_migration_bh(void *opaque)
>> @@ -1638,10 +1687,16 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>       Error *local_err = NULL;
>>       MigrationState *s = migrate_get_current();
>>       const char *p = NULL;
>> +    MigrateAddress *channel = g_new0(MigrateAddress, 1);
> Here too, use g_autoptr(MigrateAddress) and drop the  'out:' block
Ack.
>>   
>>       /* URI is not suitable for migration? */
>>       if (!migration_channels_and_uri_compatible(uri, errp)) {
>> -        return;
>> +        goto out;
>> +    }
>> +
>> +    if (!migrate_uri_parse(uri, &channel, &local_err)) {
>> +        error_setg(errp, "Error parsing uri");
>> +        goto out;
>>       }
>>   
>>       if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
>> @@ -1688,6 +1743,10 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>           error_propagate(errp, local_err);
>>           return;
>>       }
>> +
>> +out:
>> +    qapi_free_MigrateAddress(channel);
>> +    return;
>>   }
>>   
>>   void qmp_migrate_cancel(Error **errp)
>> -- 
>> 2.22.3
>>
> With regards,
> Daniel
Regards,
Het Gala