Ok, thanks!
Didn't know that trick.
On Mon, May 15, 2023, 14:17 Daniel P. Berrangé <berra...@redhat.com> wrote:
> On Mon, May 15, 2023 at 01:55:59PM +0200, Juan Quintela wrote:
> > Het Gala <het.g...@nutanix.com> wrote:
> > v> Just so that, there is a wider attention, I will try to address and
> > > discuss the comments from Daniel and Juan both here, as many of them
> > > seems to be overlapping. I hope that is fine with the maintainers.
> > >
> > > On 15/05/23 3:42 pm, Daniel P. Berrangé wrote:
> > >> On Fri, May 12, 2023 at 02:32:34PM +0000, Het Gala wrote:
> > >>> This patch introduces code that can parse 'uri' string parameter and
> > >>> spit out 'MigrateAddress' struct. All the required migration
> parameters
> > >>> are stored in the struct.
> > >>>
> > >>> Suggested-by: Aravind Retnakaran <aravind.retnaka...@nutanix.com>
> > >>> Signed-off-by: Het Gala <het.g...@nutanix.com>
> > >>> ---
> > >>> migration/migration.c | 63
> +++++++++++++++++++++++++++++++++++++++++--
> > >>> 1 file changed, 61 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/migration/migration.c b/migration/migration.c
> > >>> index 0ee07802a5..a7e4e286aa 100644
> > >>> --- a/migration/migration.c
> > >>> +++ b/migration/migration.c
> > >>> @@ -64,6 +64,7 @@
> > >>> #include "yank_functions.h"
> > >>> #include "sysemu/qtest.h"
> > >>> #include "options.h"
> > >>> +#include "qemu/sockets.h"
> > >>> static NotifierList migration_state_notifiers =
> > >>> NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> > >>> @@ -408,13 +409,58 @@ void migrate_add_address(SocketAddress
> *address)
> > >>> QAPI_CLONE(SocketAddress, address));
> > >>> }
> > >>> +static bool migrate_uri_parse(const char *uri,
> > >>> + MigrateAddress **channel,
> > >>> + Error **errp)
> > >>> +{
> > >>> + Error *local_err = NULL;
> > >>> + MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> > >>> + SocketAddress *saddr;
> > >>> + InetSocketAddress *isock = &addrs->u.rdma;
> > >>> + strList **tail = &addrs->u.exec.args;
> > >>> +
> > >>> + if (strstart(uri, "exec:", NULL)) {
> > >>> + addrs->transport = MIGRATE_TRANSPORT_EXEC;
> > >>> + QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> > >>> + QAPI_LIST_APPEND(tail, g_strdup("-c"));
> > >>> + QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
> > >>> + } else if (strstart(uri, "rdma:", NULL) &&
> > >>> + !inet_parse(isock, uri + strlen("rdma:"), errp)) {
> > >>> + addrs->transport = MIGRATE_TRANSPORT_RDMA;
> > >> I would have this as
> > >>
> > >> } else if (strstart(uri, "rdma:", NULL)) {
> > >> if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
> > >> addrs->transport = MIGRATE_TRANSPORT_RDMA;
> > >> }
> > >>
> > >> as IMHO it is bad practice to have control pass to the next
> > >> else if clause when inet_parse() fails, as we know this is
> > >> only an RDMA addr
> > > Ack. I will change in the next patch.
> > >> Also you need to use '&local_err' not 'errp' in the inet_parse
> > >> call, otherwise the later code block for cleanup won't run.
> > >
> > > Yes, thanks for pointing it out Daniel. Will modify that.
> > >
> > > Also, Juan is of the opinion that we could omit 'local_error' variable
> > > and try to address and free the memory there itself. For ex:
> > >
> > > if (saddr == NULL) {
> > > qapi_free_MigrateAddress(addrs);
> > > return false;
> > > }
> > >
> > > Or, Daniel, can I also define here the variables like you suggested
> > > down in the patch ? or is it used in some special case or I am missing
> > > something ?
> > >
> > > g_autoptr(MigrateAddress) addrs = g_new0(MigrateAddress, 1);
> > >
> > > So we would not have to worry to free MigrateAddress struct.
> >
> > https://blogs.gnome.org/desrt/2015/01/30/g_autoptr/
> >
> > Yes, but that only happens for the cases where you want to always remove
> > them.
> >
> > >>> + } else if (strstart(uri, "tcp:", NULL) ||
> > >>> + strstart(uri, "unix:", NULL) ||
> > >>> + strstart(uri, "vsock:", NULL) ||
> > >>> + strstart(uri, "fd:", NULL)) {
> > >>> + addrs->transport = MIGRATE_TRANSPORT_SOCKET;
> > >>> + saddr = socket_parse(uri, &local_err);
> > >>> + addrs->u.socket = *saddr;
> > >> Protect with
> > >>
> > >> if (saddr != NULL) {
> > >> addrs->u.socket = *saddr;
> > >> }
> > >>
> > >>> + }
> > >>> +
> > >>> + if (local_err) {
> > >>> + qapi_free_MigrateAddress(addrs);
> > >>> + qapi_free_SocketAddress(saddr);
> > >>> + qapi_free_InetSocketAddress(isock);
> > >>> + error_propagate(errp, local_err);
> > >>> + return false;
> > >>> + }
> > >>> +
> > >>> + *channel = addrs;
> > >>> + return true;
> > >>> +}
> > >>> +
> > >>> static void qemu_start_incoming_migration(const char *uri, Error
> **errp)
> > >>> {
> > >>> const char *p = NULL;
> > >>> + MigrateAddress *channel = g_new0(MigrateAddress, 1);
> > >> Avoid the later 'out:' cleanup block by using:
> > >>
> > >> g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
> > > Ack. I think this also solves the doubt raised by Juan "I wish, I
> > > really wish, that there was a way to free things on error". Am I right
> > > ?
> >
> > No, that was the case where we have something like:
> >
> > Thing *foo(void)
> > {
> > OtherThing *bar = g_new0(OtherThing, 1)
> >
> > if (whatever) {
> > goto error;
> > }
> > if (whatever_else) {
> > goto error;
> > }
> > return bar;
> > error:
> > g_free(bad);
> > return NULL;
> > }
> [>
> > See, we have to put the goto because we have to free it in all error
> > paths. Not in the non-error path.
> >
> > If it is a pure local variable, i.e. never used after the function
> > finishes, then g_autoptr is the right thing to do.
>
> 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 :|
>
>
