| Message ID | 20231004075851.219173-3-het.gala@nutanix.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration | expand |
Het Gala <het.gala@nutanix.com> wrote: > This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri' > string containing migration connection related information > and stores them inside well defined 'MigrateAddress' struct. > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> > Signed-off-by: Het Gala <het.gala@nutanix.com> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com>
Het Gala <het.gala@nutanix.com> wrote: > This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri' > string containing migration connection related information > and stores them inside well defined 'MigrateAddress' struct. > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> > Signed-off-by: Het Gala <het.gala@nutanix.com> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> queued.
Het Gala <het.gala@nutanix.com> writes: > This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri' > string containing migration connection related information > and stores them inside well defined 'MigrateAddress' struct. > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> > Signed-off-by: Het Gala <het.gala@nutanix.com> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > --- > migration/exec.c | 1 - > migration/exec.h | 4 ++++ > migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 59 insertions(+), 1 deletion(-) > > diff --git a/migration/exec.c b/migration/exec.c > index 2bf882bbe1..32f5143dfd 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); > 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 6d3cf5d5cd..dcbd509d56 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -65,6 +65,7 @@ > #include "sysemu/qtest.h" > #include "options.h" > #include "sysemu/dirtylimit.h" > +#include "qemu/sockets.h" > > static NotifierList migration_state_notifiers = > NOTIFIER_LIST_INITIALIZER(migration_state_notifiers); > @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address) > QAPI_CLONE(SocketAddress, address)); > } > > +static bool migrate_uri_parse(const char *uri, > + MigrationAddress **channel, > + Error **errp) > +{ > + g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1); This cannot be g_autoptr because you're passing it out of scope at the end of the function. > + SocketAddress *saddr = &addr->u.socket; This attribution is useless. Down below you overwrite it with the result of socket_parse. > + InetSocketAddress *isock = &addr->u.rdma; > + strList **tail = &addr->u.exec.args; > + > + if (strstart(uri, "exec:", NULL)) { > + addr->transport = MIGRATION_ADDRESS_TYPE_EXEC; > +#ifdef WIN32 > + QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path())); > + QAPI_LIST_APPEND(tail, g_strdup("/c")); > +#else > + QAPI_LIST_APPEND(tail, g_strdup("/bin/sh")); > + QAPI_LIST_APPEND(tail, g_strdup("-c")); > +#endif > + QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:"))); > + } else if (strstart(uri, "rdma:", NULL)) { > + if (inet_parse(isock, uri + strlen("rdma:"), errp)) { > + qapi_free_InetSocketAddress(isock); > + return false; > + } > + addr->transport = MIGRATION_ADDRESS_TYPE_RDMA; > + } else if (strstart(uri, "tcp:", NULL) || > + strstart(uri, "unix:", NULL) || > + strstart(uri, "vsock:", NULL) || > + strstart(uri, "fd:", NULL)) { > + addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET; > + saddr = socket_parse(uri, errp); > + if (!saddr) { > + qapi_free_SocketAddress(saddr); Shouldn't free here. socket_parse() already does so on failure. > + return false; > + } Then here you can set the values you intended to set. addr->u.socket.type = saddr->type; addr->u.socket.u = saddr->u; > + } else { > + error_setg(errp, "unknown migration protocol: %s", uri); > + return false; > + } > + > + *channel = addr; > + return true; > +} > + > static void qemu_start_incoming_migration(const char *uri, Error **errp) > { > const char *p = NULL; > + g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); The memory is leaked here because the pointer is overwritten below. Just set it to NULL. You can keep the g_autoptr. > > /* URI is not suitable for migration? */ > if (!migration_channels_and_uri_compatible(uri, errp)) { > return; > } > > + if (uri && !migrate_uri_parse(uri, &channel, errp)) { > + return; > + } > + > qapi_event_send_migration(MIGRATION_STATUS_SETUP); > if (strstart(uri, "tcp:", &p) || > strstart(uri, "unix:", NULL) || > @@ -1671,12 +1721,17 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > Error *local_err = NULL; > MigrationState *s = migrate_get_current(); > const char *p = NULL; > + g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); Same here. > > /* URI is not suitable for migration? */ > if (!migration_channels_and_uri_compatible(uri, errp)) { > return; > } > > + if (!migrate_uri_parse(uri, &channel, errp)) { > + return; > + } > + > resume_requested = has_resume && resume; > if (!migrate_prepare(s, has_blk && blk, has_inc && inc, > resume_requested, errp)) {
On Wed, Oct 04, 2023 at 11:43:12AM -0300, Fabiano Rosas wrote: > Het Gala <het.gala@nutanix.com> writes: > > > This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri' > > string containing migration connection related information > > and stores them inside well defined 'MigrateAddress' struct. > > > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> > > Signed-off-by: Het Gala <het.gala@nutanix.com> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > migration/exec.c | 1 - > > migration/exec.h | 4 ++++ > > migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 59 insertions(+), 1 deletion(-) > > > > diff --git a/migration/exec.c b/migration/exec.c > > index 2bf882bbe1..32f5143dfd 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); > > 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 6d3cf5d5cd..dcbd509d56 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -65,6 +65,7 @@ > > #include "sysemu/qtest.h" > > #include "options.h" > > #include "sysemu/dirtylimit.h" > > +#include "qemu/sockets.h" > > > > static NotifierList migration_state_notifiers = > > NOTIFIER_LIST_INITIALIZER(migration_state_notifiers); > > @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address) > > QAPI_CLONE(SocketAddress, address)); > > } > > > > +static bool migrate_uri_parse(const char *uri, > > + MigrationAddress **channel, > > + Error **errp) > > +{ > > + g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1); > > This cannot be g_autoptr because you're passing it out of scope at the > end of the function. It is still good to use g_autoptr to deal with the error paths. On the success path though you need g_steal_pointer(&addr) to prevent the autofree cleanup running. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Oct 04, 2023 at 11:43:12AM -0300, Fabiano Rosas wrote: >> Het Gala <het.gala@nutanix.com> writes: >> >> > This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri' >> > string containing migration connection related information >> > and stores them inside well defined 'MigrateAddress' struct. >> > >> > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> >> > Signed-off-by: Het Gala <het.gala@nutanix.com> >> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> >> > --- >> > migration/exec.c | 1 - >> > migration/exec.h | 4 ++++ >> > migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++ >> > 3 files changed, 59 insertions(+), 1 deletion(-) >> > >> > diff --git a/migration/exec.c b/migration/exec.c >> > index 2bf882bbe1..32f5143dfd 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); >> > 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 6d3cf5d5cd..dcbd509d56 100644 >> > --- a/migration/migration.c >> > +++ b/migration/migration.c >> > @@ -65,6 +65,7 @@ >> > #include "sysemu/qtest.h" >> > #include "options.h" >> > #include "sysemu/dirtylimit.h" >> > +#include "qemu/sockets.h" >> > >> > static NotifierList migration_state_notifiers = >> > NOTIFIER_LIST_INITIALIZER(migration_state_notifiers); >> > @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address) >> > QAPI_CLONE(SocketAddress, address)); >> > } >> > >> > +static bool migrate_uri_parse(const char *uri, >> > + MigrationAddress **channel, >> > + Error **errp) >> > +{ >> > + g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1); >> >> This cannot be g_autoptr because you're passing it out of scope at the >> end of the function. > > It is still good to use g_autoptr to deal with the error paths. > > On the success path though you need g_steal_pointer(&addr) to > prevent the autofree cleanup running. Ah good point, this has been suggested in an earlier version already, I forgot to mention. We should definitely use g_steal_pointer() whenever the variable goes out of scope.
On 10/4/2023 8:13 PM, Fabiano Rosas wrote: > Het Gala<het.gala@nutanix.com> writes: > >> This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri' >> string containing migration connection related information >> and stores them inside well defined 'MigrateAddress' struct. >> >> Suggested-by: Aravind Retnakaran<aravind.retnakaran@nutanix.com> >> Signed-off-by: Het Gala<het.gala@nutanix.com> >> Reviewed-by: Daniel P. Berrangé<berrange@redhat.com> >> --- >> migration/exec.c | 1 - >> migration/exec.h | 4 ++++ >> migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 59 insertions(+), 1 deletion(-) >> >> diff --git a/migration/exec.c b/migration/exec.c >> index 2bf882bbe1..32f5143dfd 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); >> 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 6d3cf5d5cd..dcbd509d56 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -65,6 +65,7 @@ >> #include "sysemu/qtest.h" >> #include "options.h" >> #include "sysemu/dirtylimit.h" >> +#include "qemu/sockets.h" >> >> static NotifierList migration_state_notifiers = >> NOTIFIER_LIST_INITIALIZER(migration_state_notifiers); >> @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address) >> QAPI_CLONE(SocketAddress, address)); >> } >> >> +static bool migrate_uri_parse(const char *uri, >> + MigrationAddress **channel, >> + Error **errp) >> +{ >> + g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1); > This cannot be g_autoptr because you're passing it out of scope at the > end of the function. >> + SocketAddress *saddr = &addr->u.socket; > This attribution is useless. Down below you overwrite it with the result > of socket_parse. Ack. Will assign saddr to NULL here. >> + InetSocketAddress *isock = &addr->u.rdma; >> + strList **tail = &addr->u.exec.args; >> + >> + if (strstart(uri, "exec:", NULL)) { >> + addr->transport = MIGRATION_ADDRESS_TYPE_EXEC; >> +#ifdef WIN32 >> + QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path())); >> + QAPI_LIST_APPEND(tail, g_strdup("/c")); >> +#else >> + QAPI_LIST_APPEND(tail, g_strdup("/bin/sh")); >> + QAPI_LIST_APPEND(tail, g_strdup("-c")); >> +#endif >> + QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:"))); >> + } else if (strstart(uri, "rdma:", NULL)) { >> + if (inet_parse(isock, uri + strlen("rdma:"), errp)) { >> + qapi_free_InetSocketAddress(isock); >> + return false; >> + } >> + addr->transport = MIGRATION_ADDRESS_TYPE_RDMA; >> + } else if (strstart(uri, "tcp:", NULL) || >> + strstart(uri, "unix:", NULL) || >> + strstart(uri, "vsock:", NULL) || >> + strstart(uri, "fd:", NULL)) { >> + addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET; >> + saddr = socket_parse(uri, errp); >> + if (!saddr) { >> + qapi_free_SocketAddress(saddr); > Shouldn't free here. socket_parse() already does so on failure. Okay, understood. So should just return false, nothing else here. >> + return false; >> + } > Then here you can set the values you intended to set. > > addr->u.socket.type = saddr->type; > addr->u.socket.u = saddr->u; Ack. Will do that change. >> + } else { >> + error_setg(errp, "unknown migration protocol: %s", uri); >> + return false; >> + } >> + >> + *channel = addr; >> + return true; >> +} >> + >> static void qemu_start_incoming_migration(const char *uri, Error **errp) >> { >> const char *p = NULL; >> + g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); > The memory is leaked here because the pointer is overwritten below. Just > set it to NULL. You can keep the g_autoptr. Ack. You mean it is overwritten by migrate_parse_uri function. >> >> /* URI is not suitable for migration? */ >> if (!migration_channels_and_uri_compatible(uri, errp)) { >> return; >> } >> >> + if (uri && !migrate_uri_parse(uri, &channel, errp)) { >> + return; >> + } >> + >> qapi_event_send_migration(MIGRATION_STATUS_SETUP); >> if (strstart(uri, "tcp:", &p) || >> strstart(uri, "unix:", NULL) || >> @@ -1671,12 +1721,17 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, >> Error *local_err = NULL; >> MigrationState *s = migrate_get_current(); >> const char *p = NULL; >> + g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); > Same here. Ack. Will change that. >> >> /* URI is not suitable for migration? */ >> if (!migration_channels_and_uri_compatible(uri, errp)) { >> return; >> } >> >> + if (!migrate_uri_parse(uri, &channel, errp)) { >> + return; >> + } >> + >> resume_requested = has_resume && resume; >> if (!migrate_prepare(s, has_blk && blk, has_inc && inc, >> resume_requested, errp)) { Regards, Het Gala
On 10/4/2023 11:42 PM, Fabiano Rosas wrote: > Daniel P. Berrangé<berrange@redhat.com> writes: > >> On Wed, Oct 04, 2023 at 11:43:12AM -0300, Fabiano Rosas wrote: >>> Het Gala<het.gala@nutanix.com> writes: >>> >>>> This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri' >>>> string containing migration connection related information >>>> and stores them inside well defined 'MigrateAddress' struct. >>>> >>>> Suggested-by: Aravind Retnakaran<aravind.retnakaran@nutanix.com> >>>> Signed-off-by: Het Gala<het.gala@nutanix.com> >>>> Reviewed-by: Daniel P. Berrangé<berrange@redhat.com> >>>> --- >>>> migration/exec.c | 1 - >>>> migration/exec.h | 4 ++++ >>>> migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 59 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/migration/exec.c b/migration/exec.c >>>> index 2bf882bbe1..32f5143dfd 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); >>>> 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 6d3cf5d5cd..dcbd509d56 100644 >>>> --- a/migration/migration.c >>>> +++ b/migration/migration.c >>>> @@ -65,6 +65,7 @@ >>>> #include "sysemu/qtest.h" >>>> #include "options.h" >>>> #include "sysemu/dirtylimit.h" >>>> +#include "qemu/sockets.h" >>>> >>>> static NotifierList migration_state_notifiers = >>>> NOTIFIER_LIST_INITIALIZER(migration_state_notifiers); >>>> @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address) >>>> QAPI_CLONE(SocketAddress, address)); >>>> } >>>> >>>> +static bool migrate_uri_parse(const char *uri, >>>> + MigrationAddress **channel, >>>> + Error **errp) >>>> +{ >>>> + g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1); >>> This cannot be g_autoptr because you're passing it out of scope at the >>> end of the function. >> It is still good to use g_autoptr to deal with the error paths. >> >> On the success path though you need g_steal_pointer(&addr) to >> prevent the autofree cleanup running. > Ah good point, this has been suggested in an earlier version already, I > forgot to mention. We should definitely use g_steal_pointer() whenever > the variable goes out of scope. Okay. I get the point that g_autoptr helps to deal with freeing of pointer in case error occurs inside the function. But I am still trying to figure out why we need g_steal_pointer() ? If you could please explain once again. My understanding till now was that if we want to return 'addr' variable as return type, then we would want to make use of g_steal_pointer(&addr) so instead of addr, we pass a temp ptr refrencing to the same location as addr, and then assign addr = NULL. But we are already assigning the memory location where addr was refrencing to 'channel'. Let me know if I am missing something ? I think the syntax follows as the second example given here: https://docs.gtk.org/glib/func.steal_pointer.html ? Regards, Het Gala
Het Gala <het.gala@nutanix.com> writes: > On 10/4/2023 11:42 PM, Fabiano Rosas wrote: >> Daniel P. Berrangé<berrange@redhat.com> writes: >> >>> On Wed, Oct 04, 2023 at 11:43:12AM -0300, Fabiano Rosas wrote: >>>> Het Gala<het.gala@nutanix.com> writes: >>>> >>>>> This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri' >>>>> string containing migration connection related information >>>>> and stores them inside well defined 'MigrateAddress' struct. >>>>> >>>>> Suggested-by: Aravind Retnakaran<aravind.retnakaran@nutanix.com> >>>>> Signed-off-by: Het Gala<het.gala@nutanix.com> >>>>> Reviewed-by: Daniel P. Berrangé<berrange@redhat.com> >>>>> --- >>>>> migration/exec.c | 1 - >>>>> migration/exec.h | 4 ++++ >>>>> migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 59 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/migration/exec.c b/migration/exec.c >>>>> index 2bf882bbe1..32f5143dfd 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); >>>>> 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 6d3cf5d5cd..dcbd509d56 100644 >>>>> --- a/migration/migration.c >>>>> +++ b/migration/migration.c >>>>> @@ -65,6 +65,7 @@ >>>>> #include "sysemu/qtest.h" >>>>> #include "options.h" >>>>> #include "sysemu/dirtylimit.h" >>>>> +#include "qemu/sockets.h" >>>>> >>>>> static NotifierList migration_state_notifiers = >>>>> NOTIFIER_LIST_INITIALIZER(migration_state_notifiers); >>>>> @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address) >>>>> QAPI_CLONE(SocketAddress, address)); >>>>> } >>>>> >>>>> +static bool migrate_uri_parse(const char *uri, >>>>> + MigrationAddress **channel, >>>>> + Error **errp) >>>>> +{ >>>>> + g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1); >>>> This cannot be g_autoptr because you're passing it out of scope at the >>>> end of the function. >>> It is still good to use g_autoptr to deal with the error paths. >>> >>> On the success path though you need g_steal_pointer(&addr) to >>> prevent the autofree cleanup running. >> Ah good point, this has been suggested in an earlier version already, I >> forgot to mention. We should definitely use g_steal_pointer() whenever >> the variable goes out of scope. > Okay. I get the point that g_autoptr helps to deal with freeing of > pointer in case error occurs inside the function. Yes, but note g_autoptr will free the memory any time the variable goes out of scope, i.e. any time we return from the function. Even when there's no error and we actually want that memory to still be around for the callers to use. > But I am still trying to figure out why we need g_steal_pointer() ? If > you could please explain once again. > My understanding till now was that if we want to return 'addr' variable > as return type, then we would want to make use of g_steal_pointer(&addr) > so instead of addr, we pass a temp ptr refrencing to the same location > as addr, and then assign addr = NULL. But we are already assigning the > memory location where addr was refrencing to 'channel'. Let me know if I > am missing something ? So now 'channel' points to the memory you allocated at the start of the function with g_new0. When the function returns, g_autoptr has no idea that someone is still using that memory, so it will just free it. Whenever you want a chunk of memory to be accessed across function calls like that you need to steal the reference. After stealing, the pointer that was registered with g_autoptr (in this case 'addr') now points to nothing and the pointer that was returned/assigned is a different one that isn't known by any cleanup functions. Note that after g_steal_pointer, that memory now becomes responsibility of whatever piece of code owns that pointer. In this case, qemu_start_incoming_migration() and qmp_migrate(). Those two functions will have to free the memory once they are done with it. Or do the g_autoptr/g_steal_pointer trick once more. > I think the syntax follows as the second example given here: > https://docs.gtk.org/glib/func.steal_pointer.html ? Yep, that's it.
On 10/9/2023 7:43 PM, Fabiano Rosas wrote: > Het Gala<het.gala@nutanix.com> writes: > >> On 10/4/2023 11:42 PM, Fabiano Rosas wrote: >>> Daniel P. Berrangé<berrange@redhat.com> writes: >>> >>>> On Wed, Oct 04, 2023 at 11:43:12AM -0300, Fabiano Rosas wrote: >>>>> Het Gala<het.gala@nutanix.com> writes: >>>>> >>>>>> This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri' >>>>>> string containing migration connection related information >>>>>> and stores them inside well defined 'MigrateAddress' struct. >>>>>> >>>>>> Suggested-by: Aravind Retnakaran<aravind.retnakaran@nutanix.com> >>>>>> Signed-off-by: Het Gala<het.gala@nutanix.com> >>>>>> Reviewed-by: Daniel P. Berrangé<berrange@redhat.com> >>>>>> --- >>>>>> migration/exec.c | 1 - >>>>>> migration/exec.h | 4 ++++ >>>>>> migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++ >>>>>> 3 files changed, 59 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/migration/exec.c b/migration/exec.c >>>>>> index 2bf882bbe1..32f5143dfd 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); >>>>>> 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 >>>>>> rate_add_address(SocketAddress *address) >>>>>> ng it out of scope at the >>>>>> end of the function. >>>> It is still good to use g_autoptr to deal with the error paths. >>>> >>>> On the success path though you need g_steal_pointer(&addr) to >>>> prevent the autofree cleanup running. >>> Ah good point, this has been suggested in an earlier version already, I >>> forgot to mention. We should definitely use g_steal_pointer() whenever >>> the variable goes out of scope. >> Okay. I get the point that g_autoptr helps to deal with freeing of >> pointer in case error occurs inside the function. > Yes, but note g_autoptr will free the memory any time the variable goes > out of scope, i.e. any time we return from the function. Even when > there's no error and we actually want that memory to still be around for > the callers to use. > >> But I am still trying to figure out why we need g_steal_pointer() ? If >> you could please explain once again. >> My understanding till now was that if we want to return 'addr' variable >> as return type, then we would want to make use of g_steal_pointer(&addr) >> so instead of addr, we pass a temp ptr refrencing to the same location >> as addr, and then assign addr = NULL. But we are already assigning the >> memory location where addr was refrencing to 'channel'. Let me know if I >> am missing something ? > So now 'channel' points to the memory you allocated at the start of the > function with g_new0. When the function returns, g_autoptr has no idea > that someone is still using that memory, so it will just free it. > > Whenever you want a chunk of memory to be accessed across function calls > like that you need to steal the reference. After stealing, the pointer > that was registered with g_autoptr (in this case 'addr') now points to > nothing and the pointer that was returned/assigned is a different one > that isn't known by any cleanup functions. > > Note that after g_steal_pointer, that memory now becomes responsibility > of whatever piece of code owns that pointer. In this case, > qemu_start_incoming_migration() and qmp_migrate(). Those two functions > will have to free the memory once they are done with it. Or do the > g_autoptr/g_steal_pointer trick once more. Got your point perfectly now. Thanks a lot. Just so that I understood right, summarising it in short: g_autoptr --> helps in error paths so we use it, but for non-error paths if it goes out of scope, it will free it so to prevent that we have to use g_steal_pointer(). Either use g_autoptr/g_steal_pointer pair or do it simple but then will have to take care of error paths as well as non-error paths to free the pointers. >> I think the syntax follows as the second example given here: >> https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.gtk.org_glib_func.steal-5Fpointer.html&d=DwIFaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=Fn2ZhrrsSvcV_ZUX3PUnI1zMMD7JM5o1X9LbURlU1B7O06RNnzmOS3hKI4HBe6fB&s=iJFFbZnxzfqd1xu3gRVfvdjjE_X3XT3t0aRRAIeaUUc&e= ? > Yep, that's it. Ack. Regards, Het Gala
diff --git a/migration/exec.c b/migration/exec.c index 2bf882bbe1..32f5143dfd 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); 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 6d3cf5d5cd..dcbd509d56 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -65,6 +65,7 @@ #include "sysemu/qtest.h" #include "options.h" #include "sysemu/dirtylimit.h" +#include "qemu/sockets.h" static NotifierList migration_state_notifiers = NOTIFIER_LIST_INITIALIZER(migration_state_notifiers); @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address) QAPI_CLONE(SocketAddress, address)); } +static bool migrate_uri_parse(const char *uri, + MigrationAddress **channel, + Error **errp) +{ + g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1); + SocketAddress *saddr = &addr->u.socket; + InetSocketAddress *isock = &addr->u.rdma; + strList **tail = &addr->u.exec.args; + + if (strstart(uri, "exec:", NULL)) { + addr->transport = MIGRATION_ADDRESS_TYPE_EXEC; +#ifdef WIN32 + QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path())); + QAPI_LIST_APPEND(tail, g_strdup("/c")); +#else + QAPI_LIST_APPEND(tail, g_strdup("/bin/sh")); + QAPI_LIST_APPEND(tail, g_strdup("-c")); +#endif + QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:"))); + } else if (strstart(uri, "rdma:", NULL)) { + if (inet_parse(isock, uri + strlen("rdma:"), errp)) { + qapi_free_InetSocketAddress(isock); + return false; + } + addr->transport = MIGRATION_ADDRESS_TYPE_RDMA; + } else if (strstart(uri, "tcp:", NULL) || + strstart(uri, "unix:", NULL) || + strstart(uri, "vsock:", NULL) || + strstart(uri, "fd:", NULL)) { + addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET; + saddr = socket_parse(uri, errp); + if (!saddr) { + qapi_free_SocketAddress(saddr); + return false; + } + } else { + error_setg(errp, "unknown migration protocol: %s", uri); + return false; + } + + *channel = addr; + return true; +} + static void qemu_start_incoming_migration(const char *uri, Error **errp) { const char *p = NULL; + g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); /* URI is not suitable for migration? */ if (!migration_channels_and_uri_compatible(uri, errp)) { return; } + if (uri && !migrate_uri_parse(uri, &channel, errp)) { + return; + } + qapi_event_send_migration(MIGRATION_STATUS_SETUP); if (strstart(uri, "tcp:", &p) || strstart(uri, "unix:", NULL) || @@ -1671,12 +1721,17 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, Error *local_err = NULL; MigrationState *s = migrate_get_current(); const char *p = NULL; + g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1); /* URI is not suitable for migration? */ if (!migration_channels_and_uri_compatible(uri, errp)) { return; } + if (!migrate_uri_parse(uri, &channel, errp)) { + return; + } + resume_requested = has_resume && resume; if (!migrate_prepare(s, has_blk && blk, has_inc && inc, resume_requested, errp)) {