Re: [PATCH 1/5] migration: Updated QAPI format for 'migrate' qemu monitor command

[Daniel P . Berrangé]

On Tue, Jan 10, 2023 at 01:09:35PM +0530, Het Gala wrote:
> 
> On 09/01/23 7:37 pm, Daniel P. Berrangé wrote:
> > On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote:
> > > From: Author Het Gala <het.gala@nutanix.com>
> > > 
> > > Existing 'migrate' QAPI design enforces transport mechanism, ip address
> > > of destination interface and corresponding port number in the form
> > > of a unified string 'uri' parameter. This scheme does seem to have an 
> > > issue
> > > in it, i.e. double-level encoding of URIs.
> > > 
> > > The current patch maps existing QAPI design into a well-defined data
> > > structure - 'MigrateChannel' only from the design perspective. Please 
> > > note that
> > > the existing 'uri' parameter is kept untouched for backward compatibility.
> > > 
> > > +##
> > > +# @MigrateExecAddr:
> > > + #
> > > + # Since 8.0
> > > + ##
> > > +{ 'struct': 'MigrateExecAddr',
> > > +   'data' : {'exec-str': 'str' } }
> > Currently for 'exec:cmdstr' the 'cmdstr' part is a shell command
> > that is passed
> > 
> >    const char *argv[] = { "/bin/sh", "-c", command, NULL };
> > 
> > I have a strong preference for making it possible to avoid use
> > of shell when spawning commands, since much of thue time it is
> > not required and has the potential to open up vulnerabilities.
> > It would be nice to be able to just take the full argv directly
> > IOW
> > 
> >   { 'struct': 'MigrateExecAddr',
> >      'data' : {'argv': ['str'] } }
> > 
> > If the caller wants to keep life safe and simple now they can
> > use
> >     ["/bin/nc", "-U", "/some/sock"]
> > 
> > but if they still want to send it via shell, they can also do
> > so
> > 
> >     ["/bin/sh", "-c", "...arbitrary shell script code...."]
> Okay Daniel. I get your point and it makes sense too. This will also have
> some code changes in exec.c file I assume.
> > > +
> > > +##
> > > +# @MigrateRdmaAddr:
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'struct': 'MigrateRdmaAddr',
> > > +   'data' : {'rdma-str': 'str' } }
> > Loooking at the RDMA code it takes the str, and treats it
> > as an IPv4 address:
> > 
> > 
> >          addr = g_new(InetSocketAddress, 1);
> >          if (!inet_parse(addr, host_port, NULL)) {
> >              rdma->port = atoi(addr->port);
> >              rdma->host = g_strdup(addr->host);
> >              rdma->host_port = g_strdup(host_port);
> >          }
> > 
> > so we really ought to accept an InetSocketAddress struct
> > directly
> > 
> >   { 'struct': 'MigrateRdmaAddr',
> >      'data' : {'rdma-str': 'InetSocketAddress' } }
> > 
> Yes, It resembles to InetSocketAddress. Will make the relevant changes in
> rdma.c file.
> 
> With this, I had a small question in mind, do qemu need to develop /
> leverage some functionality to check the correctness for host or port.
> So that if the user enters an invalid host address, they get an error
> message to enter correct address, if trying to migrate via qmp command line
> interface.

When the RDMA code uses the host address to resolve the RDMA
endpoint, it will fail and report an error back.


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