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

[Stefan Hajnoczi]

On Thu, Mar 05, 2020 at 08:21:14AM +0000, Daniel P. Berrangé wrote:
> 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 <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.
> > > > >
> > > > 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.

Not all of the remote program's command-line options may be known to
QEMU, so passing through just a few options like -drive does not fix the
problem.

I suggest just providing the arguments as a single string parameter like
this patch series already does, but make sure to consider the escaping
rules for commas and other special characters.

Stefan

signature.asc
Description: PGP signature