Het Gala <het.g...@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.retnaka...@nutanix.com>
>>> Signed-off-by: Het Gala <het.g...@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.
