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

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
v> 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.

https://blogs.gnome.org/desrt/2015/01/30/g_autoptr/

Yes, but that only happens for the cases where you want to always remove
them.

>>> +    } 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
> ?

No, that was the case where we have something like:

Thing *foo(void)
{
    OtherThing *bar = g_new0(OtherThing, 1)

    if (whatever) {
        goto error;
    }
    if (whatever_else) {
        goto error;
    }
    return bar;
error:
    g_free(bad);
    return NULL;
}

See, we have to put the goto because we have to free it in all error
paths.  Not in the non-error path.

If it is a pure local variable, i.e. never used after the function
finishes, then g_autoptr is the right thing to do.

Later, Juan.