| Message ID | 20230512143240.192504-3-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Modified 'migrate' and 'migrate-incoming' QAPI commands for migration | expand |
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)
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 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
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
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 :| > >
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)
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(-)