Re: [PATCH v4 3/8] migration: converts socket backend to accept MigrateAddress struct

[Daniel P . Berrangé]

On Mon, May 15, 2023 at 09:58:37PM +0530, Het Gala wrote:
> 
> On 15/05/23 8:46 pm, Het Gala wrote:
> > 
> > On 15/05/23 8:16 pm, Juan Quintela wrote:
> > > Het Gala <het.gala@nutanix.com> wrote:
> > > > On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
> > > > > On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
> > > > > > Socket transport backend for
> > > > > > 'migrate'/'migrate-incoming' QAPIs accept
> > > > > > new wire protocol of MigrateAddress struct.
> > > > > > 
> > > > > > It is achived by parsing 'uri' string and storing
> > > > > > migration parameters
> > > > > > required for socket connection into well defined
> > > > > > SocketAddress struct.
> > > > > > 
> > > > > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> > > > > > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > > > > > ---
> > > > > >    migration/exec.c      |  4 ++--
> > > > > >    migration/exec.h      |  4 ++++
> > > > > >    migration/migration.c | 44
> > > > > > +++++++++++++++++++++++++++++++------------
> > > > > >    migration/socket.c    | 34 +++++----------------------------
> > > > > >    migration/socket.h    |  7 ++++---
> > > > > >    5 files changed, 47 insertions(+), 46 deletions(-)
> > > > > > 
> > > > > > diff --git a/migration/exec.c b/migration/exec.c
> > > > > > index 2bf882bbe1..c4a3293246 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)
> Maintainers please advice. If I want to see thatthe build is proper, how to
> enable WIN32 support on a CentOS guest operating system (what all
> dependencies to install, what to configure for a build which supports WIN32)
> ? Any pointers ?

QEMU does Windows build testing using Fedora, which ships mingw
packages. If you only have centos, then use docker/podman to
get a Fedora environment in a container locally.

Alternatively push to gitlab and run a CI pipeline (see docs/devel/ci.rst
for more info)

> > > > Even this, I will shift it to the 2nd patch, where I need to move
> > > > exec_get_cmd_path() out accross other file (migration.c).
> > > great.
> > > 
> > > > > >    {
> > > > > >        g_autofree char *detected_path = g_new(char, MAX_PATH);
> > > > > > @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
> > > > > >    }
> > > > > >    #endif
> > > > > >    -void exec_start_outgoing_migration(MigrationState *s, const char
> > > > > > *command, Error **errp)
> > > > > > +void exec_start_outgoing_migration(MigrationState *s,
> > > > > > const char *command,
> > > > > > +                                   Error **errp)
> > > > > >    {
> > > > > >        QIOChannel *ioc;
> > > > Sure, Juan. Will change this in the 2nd patch itself instead of
> > > > here. I am not very convinved why should we have a different patch all
> > > > together for this, because we are just using this code outside this
> > > > file in my opinion? But if you still think so, I can make a different
> > > > patch for that.
> > > It is up to you.
> > 
> > For now, I will keep it with 2nd patch. If any other maintainer also
> > thinks that it should be a separate patch all together of windows
> > support for exec, I will make a new patch for that. But thankyou for
> > your suggestion 😁
> > 
> > > > Juan, I get your point. But I think, we won't be needing local_err at
> > > > all, if I use g_autoptr for 'channel' and 'saddr' is a part of
> > > > 'channel'. Let me have a v2 patchset and if it is still not
> > > > convinving, we can have a discussion on this.
> > > > > THis leaks 'channel', and free's 'saddr' which actually  belongs
> > > > > to channel.
> > > > > 
> > > > > With my comments on the previous patch suggesting g_autoptr for
> > > > > 'channel', we don't need any free calls for 'saddr' or 'channel'.
> > > > Right. With g_autoptr used for freeing 'channel' in last patch, we
> > > > wont have to worry about freeing 'saddr' at all. Thanks Daniel
> > > > 
> > > > if (local_err) {
> > > >      qapi_free_SocketAddress(saddr);
> > > >      error_propagate(errp, local_err);
> > > >      return;
> > > > }
> > > > And after changing the position for assigning 'saddr' and using
> > > > g_autoptr for 'channel' I believe we can get rid of 'local_error'
> > > > variable too and replace it with 'errp'. Please suggest if I am
> > > > missing something here. TIA!
> > > great.  That is much better.
> > Ack.
> > > > > >    -void socket_start_outgoing_migration(MigrationState *s,
> > > > > > -                                     const char *str,
> > > > > > -                                     Error **errp)
> > > > > > -{
> > > > > > -    Error *err = NULL;
> > > > > > -    SocketAddress *saddr = socket_parse(str, &err);
> > > > > > -    if (!err) {
> > > > > > -        socket_start_outgoing_migration_internal(s, saddr, &err);
> > > > > > -    }
> > > > > > -    error_propagate(errp, err);
> > > > > > -}
> > > > > > -
> > > > Actually Juan. I don't need this function at all, because parsing of
> > > > uri into socketAddress using socket_parse is already done. So there is
> > > > no use of having this function in the first place, so I decided to
> > > > delete this fucntion all together. Same with incoming function.
> > > What I mean is that this code was already there.  And it was doing it
> > > wrong.  The parts that I corrected you were using this pattern, chcking
> > > that err was NULL, intsead of cheking that saddr is NULL.
> > Yes, I get your point. But that function is useless if socket_parse()
> > function is not needed. So have omitted the function all together as
> > socket parsing is already done in earlier patches.
> > > Later, Juan.
> > Regards,
> > Het Gala
> 

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 :|