Re: [PATCH v5 14/50] mutli-process: build remote command line args

[Daniel P . Berrangé]

On Mon, Mar 02, 2020 at 02:39:37PM -0800, Elena Ufimtseva wrote:
> On Mon, Mar 02, 2020 at 05:47:45PM +0000, Daniel P. Berrangé wrote:
> > On Mon, Mar 02, 2020 at 06:36:13PM +0100, Philippe Mathieu-Daudé wrote:
> > > typo "multi" in patch subject.
> > >
> Thank Philippe, will fix.
>  
> > > On 2/24/20 9:55 PM, Jagannathan Raman wrote:
> > > > From: Elena Ufimtseva <address@hidden>
> > > > 
> > > > Signed-off-by: Elena Ufimtseva <address@hidden>
> > > > Signed-off-by: Jagannathan Raman <address@hidden>
> > > > Signed-off-by: John G Johnson <address@hidden>
> > > > ---
> > > >   v4 -> v5:
> > > >    - Added "exec" suboption to get the executable's name
> > > >    - Addressed feedback about variable names
> > > >    - Removed redundant check for spawning a process
> > > > 
> > > >   hw/proxy/qemu-proxy.c         | 68 
> > > > +++++++++++++++++++++++++++++++++----------
> > > >   include/hw/proxy/qemu-proxy.h |  2 +-
> > > >   2 files changed, 54 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/hw/proxy/qemu-proxy.c b/hw/proxy/qemu-proxy.c
> > > > index 828bbd7..d792e86 100644
> > > > --- a/hw/proxy/qemu-proxy.c
> > > > +++ b/hw/proxy/qemu-proxy.c
> > > > @@ -19,19 +19,50 @@
> > > >   static void pci_proxy_dev_realize(PCIDevice *dev, Error **errp);
> > > > +static int add_argv(char *opts_str, char **argv, int argc)
> > > > +{
> > > > +    int max_args = 64;
> > > > +
> > > > +    if (argc < max_args - 1) {
> > > > +        argv[argc++] = opts_str;
> > > > +        argv[argc] = 0;
> > > > +    } else {
> > > > +        return 0;
> > > > +    }
> > > > +
> > > > +    return argc;
> > > > +}
> > > > +
> > > > +static int make_argv(char *opts_str, char **argv, int argc)
> > > > +{
> > > > +    int max_args = 64;
> > > > +
> > > > +    char *p2 = strtok(opts_str, " ");
> > > > +    while (p2 && argc < max_args - 1) {
> > > > +        argv[argc++] = p2;
> > > > +        p2 = strtok(0, " ");
> > > > +    }
> > > > +    argv[argc] = 0;
> > > 
> > > Is there a GLib function to do that?
> >
> 
> Hi Daniel
> 
> > g_shell_parse_argv() perhaps
> >
> 
> Thanks for the suggestion.
> 
> >   https://developer.gnome.org/glib/stable/glib-Shell-related-Utilities.html
> > 
> > 
> > Though my preference would be to avoid the need to do this at all, by
> > not accepting a raw shell command line string in the first place.
> >
> Can you please clarify? Did you mean that it would be better if Qemu somehow
> verifies the options and then passes it to a remote process via a message?

I've not been able to trace the code paths back all the way, so I can't
point to where I think needs fixing. I assuming that something, somewhere
in this patch series should starts out with a binary name and a list of argv
as an array of char *. ie a "char **argv".  At some point this array gets
mashed together into a single 'char *' string where all the argv are separated
by a space. This patch now tries to parse this and turn it back into a
"char **argv" array.

So my key point is that we should try hard to avoid this intermediate
shell command line string stage entirely. Always keep the argv in an array
form, and never mash them together such that they then need parsing again.

I understand this is probably more complex, because we're having to pass
this across processes, via QemuOpts IIUC, but I still believe it is important
to have this data kept in array format if at all practical.

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