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;
+ } 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;
+ }
+
+ 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);
/* 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");
+ 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);
/* 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.3On 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
Also you need to use '&local_err' not 'errp' in the inet_parse
call, otherwise the later code block for cleanup won't run.
> + } 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);
>
> /* 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.
> + 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
>
> /* 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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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
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.
On Mon, May 15, 2023 at 01:55:59PM +0200, Juan Quintela wrote:
> 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.
It is still better to use g_autoptr even in that case. You just need
to add in a call to g_steal_pointer in the success path. eg
Thing *foo(void)
{
g_autoptr(OtherThing) bar = g_new0(OtherThing, 1)
if (whatever) {
return NULL;
}
if (whatever_else) {
return NULL;
}
return g_steal_pointer(&bar);
}
g_steal_pointer(&bar) is the equivalent of doing
OtherThing *tmp = bar;
bar = NULL;
return tmp;
thus avoiding free'ing the pointer you're returning
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Ok, thanks!
Didn't know that trick.
On Mon, May 15, 2023, 14:17 Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Mon, May 15, 2023 at 01:55:59PM +0200, Juan Quintela wrote:
> > 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.
>
> It is still better to use g_autoptr even in that case. You just need
> to add in a call to g_steal_pointer in the success path. eg
>
> Thing *foo(void)
> {
> g_autoptr(OtherThing) bar = g_new0(OtherThing, 1)
>
> if (whatever) {
> return NULL;
> }
> if (whatever_else) {
> return NULL;
> }
> return g_steal_pointer(&bar);
> }
>
>
> g_steal_pointer(&bar) is the equivalent of doing
>
>
> OtherThing *tmp = bar;
> bar = NULL;
> return tmp;
>
> thus avoiding free'ing the pointer you're returning
>
> With regards,
> Daniel
> --
> |: https://berrange.com -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o-
> https://www.instagram.com/dberrange :|
>
>
Het Gala <het.gala@nutanix.com> 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>
Markus, could you comment?
> ---
> 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;
Remove this.
> + 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;
> + } 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);
saddr = socket_parse(uri, &local_err);
if (saddr == NULL) {
qapi_free_MigrateAddress(addrs);
return false;
}
> + addrs->u.socket = *saddr;
> + }
> +
> + if (local_err) {
> + qapi_free_SocketAddress(saddr);
> + qapi_free_InetSocketAddress(isock);
> + error_propagate(errp, local_err);
> + return false;
> + }
And remove this last chunk?
Or I am missing something?
> +
> + *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);
>
> /* 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");
Why are we doing this error_setg? We are supposed to have a better
error assigned to errp already, no?
> + 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);
I wish, I really wish, that there was a way to free things on error.
> }
>
> 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);
>
> /* 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");
Same here about setting errp.
> + 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)
© 2016 - 2026 Red Hat, Inc.