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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Mar 04, 2020 at 02:37:43PM -0800, Elena Ufimtseva wrote:
> On Wed, Mar 04, 2020 at 04:33:57PM +0000, Daniel P. Berrangé wrote:
> > On Wed, Mar 04, 2020 at 08:25:34AM -0800, Elena Ufimtseva wrote:
> > > On Wed, Mar 04, 2020 at 11:00:32AM +0000, Daniel P. Berrangé wrote:
> > > > 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 <elena.ufimtseva@oracle.com>
> > > > > > > > 
> > > > > > > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > > > > > > > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> > > > > > > > Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> > > > > > > > ---
> > > > > > > >   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.
> > > >
> > > Hi Daniel
> > > 
> > > Thank you for explanation.
> > > At this point there is no intermediate stage as we grab the arguments
> > > as a raw string from the command line option -remote:
> > > 
> > > -remote rid=8,exec=qemu-scsi-dev,command="-drive id=drive_image2,,file=/root/remote-process-disk.img"
> > > 
> > > So the command="" string is being later parsed into the array and remote process
> > > gets spawned with the "char **argv".
> > > 
> > > Stefan expressed his concern that its not convenient to use due to
> > > the double escaping commas, spaces, quotes and we do agree with that.
> > > We were seeking an advice on what is the better approach.
> > 
> > I've not looked closely enough to understand the range of different
> > options we need to be able to pass to the remote QEMU ? Is it just
> > "-drive" options, or can it be absolutely any QEMU option ?
> > 
> > If it is just -drive, then I could imagine a -remote-drive option
> > such that we end up with with a set of args
> > 
> >    $QEMU \
> >    -remote rid=8,exec=qemu-scsi-dev \
> >    -remote-drive rid=8,id=drive_image1,file=/root/remote-process-disk1.img \
> >    -remote-drive rid=8,id=drive_image2,file=/root/remote-process-disk2.img \
> >    -remote rid=9,exec=qemu-scsi-dev \
> >    -remote-drive rid=9,id=drive_image3,file=/root/remote-process-disk3.img \
> >    -remote-drive rid=9,id=drive_image4,file=/root/remote-process-disk4.img
> > 
> > And this gets turned into 2 execs:
> > 
> >    qemu-scsi-dev \
> >    -drive rid=8,id=drive_image1,file=/root/remote-process-disk1.img \
> >    -drive rid=8,id=drive_image2,file=/root/remote-process-disk2.img
> >    
> >    qemu-scsi-dev \
> >    -drive rid=9,id=drive_image3,file=/root/remote-process-disk3.img \
> >    -drive rid=9,id=drive_image4,file=/root/remote-process-disk4.img
> > 
> > 
> > Or maybe instead of having a '-remote-drive' arg, we can make the '-drive'
> > arg take an optional "rid" attribute to associate it with the remote process
> > 
> >    $QEMU \
> >    -remote rid=8,exec=qemu-scsi-dev \
> >    -drive rid=8,id=drive_image1,file=/root/remote-process-disk1.img \
> >    -drive rid=8,id=drive_image2,file=/root/remote-process-disk2.img \
> >    -remote rid=9,exec=qemu-scsi-dev \
> >    -drive rid=9,id=drive_image3,file=/root/remote-process-disk3.img \
> >    -drive rid=9,id=drive_image4,file=/root/remote-process-disk4.img
> > 
> > When 'rid' is seen, instead of creating a local block backend, the
> > args get used for the remote process.
> > 
> > This would have the nice user behaviour that you can take an existing
> > QEMU command line, and turn it into a multi-process command line
> > simply by adding the '-remote ...' arg, and adding 'rid=NN' to
> > each -drive. Nothing else about your existing command line need
> > change.
> 
> This does look good, especially unmodified -drive.
> And -monitor for the remote process could also be similarly added
> with only rid specified instead of plugging it into the raw string.
> 
> Stefan did mention in the another patch that he thinks that adding
> -remote option is too invasive and suggested using -object itself
> to further separate remote process devices.
> 
> So to compile both replies, the command line for the remote process
> will look something like this:
> 
> 
> -object remote-device,id=rid0,exec=qemu-scsi-dev \
> -device remote-pci-device,id=scsi0,remote-device=rid0 \
> -device scsi-hd,drive=drive_image1,bus=scsi0.0,scsi-id=0,remote-device=rid0 \
> -drive id=drive_image3,file=/root/remote-process-disk3.img,remote-device=rid0 \
> -drive id=drive_image4,file=/root/remote-process-disk4.img,remote-device=rid0 \
> -monitor unix:/home/qmp-sock,,server,,nowait,remote-device=rid0
> 
> And in experimental version we imply that remote-pci-device is the LSI controller.
> For vfio-over-socket it will represent any remote PCI device.
> 
> What your thoughts on this?

Looks like a reasonable idea to me. I guess I'm not sure how much the block
maintainers will like having a -drive additional property. Probably depends
how much it impacts the code paths processing it.


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