Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.

[Het Gala]

On 30/05/23 5:40 pm, Daniel P. Berrangé wrote:
> On Tue, May 30, 2023 at 01:02:27PM +0530, Het Gala wrote:
> > On 30/05/23 12:28 pm, Markus Armbruster wrote:
> > > Het Gala<het.gala@nutanix.com>  writes:
[...]
> > > > > > +##
> > > > > > +{ 'enum': 'MigrateTransport',
> > > > > > +  'data': ['socket', 'exec', 'rdma'] }
> > > > > > +
> > > > > > +##
> > > > > > +# @MigrateExecCommand:
> > > > > Documentation of @args is missing.
> > > > Ack. Should the naming '@args' be replaced by '@filepath' or @path' or 
> > > > something similar ?
> > > Depends on what @args means.
> > >
> > > I guess its [program, arg1, arg2, ...].
> Yes, that is correct. Essentially this ends up calling
>
>     execve(@args[0], @args)
>
> > > You could split off the program:
> > >
> > >       'program: 'str',
> > >       'args': [ 'str' ]
> Now you also have to declare whether '@args' includes or excludes
> 'program' as @args[0]. execve() style would be to have '@args[0]'
> duplicate 'program'. Personally I think that's overkill for QEMU's
> needs. If we don't include 'program' in @args, then we have to
> document this, as it isn't discoverable from the QAPI design.
>
> Not separating 'program' and 'args' in QAPI makes it unambiguous
> that 'args' must include everything.
Yes, we need to make user aware of args[0], so keeping it @args along 
with adding a note that @args[0] - path to the new program ? is the best 
alternative here ? - Markus, Daniel
> > > Try to write clear documentation for both alternatives.  Such an
> > > exercise tends to lead me to the one I prefer.
> > Hmm, basically here the @args means, for example ['/bin/bash', args1, args2,
> > ..., <command>], where command -> /some/file/path.
> >
> > Does it even make sense now to break into 3 different parts ?
> >
> >         'program': 'str'
> >         'args': [ 'str' ]
> >         'command': 'str'
> Definitely not. This encodes an assumption that we're spawning via a
> shell. The intent with the new design is that it lets mgmt apps fully
> eliminate use of shell, and directly invoke the program, thus eliminating
> potential (security) pitfalls with shell metacharacters.
Got your point Daniel. Thanks.
> With regards,
> Daniel
Regards,
Het Gala