| Message ID | 588dafeecd20f8562f4a0dd68fa4bafbd6ea18bb.1582576372.git.jag.raman@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | Initial support for multi-process qemu | expand |
typo "multi" in patch subject. 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? > + > + return argc; > +} > + > static int remote_spawn(PCIProxyDev *pdev, const char *opts, > const char *exec_name, Error **errp) > { > - char *args[3]; > pid_t rpid; > int fd[2] = {-1, -1}; > Error *local_error = NULL; > + char *argv[64]; > + int argc = 0; > + char *sfd; > + char *exec_dir; > int rc = -EINVAL; > > if (pdev->managed) { > /* Child is forked by external program (such as libvirt). */ > error_setg(errp, "Remote processed is managed and launched by external program"); > - return -1; > + return rc; > } > > if (!exec_name) { > @@ -41,32 +72,38 @@ static int remote_spawn(PCIProxyDev *pdev, const char *opts, > > if (socketpair(AF_UNIX, SOCK_STREAM, 0, fd)) { > error_setg(errp, "Unable to create unix socket."); > - return -1; > + return rc; > } > + exec_dir = g_strdup_printf("%s/%s", qemu_get_exec_dir(), exec_name); > + argc = add_argv(exec_dir, argv, argc); > + sfd = g_strdup_printf("%d", fd[1]); > + argc = add_argv(sfd, argv, argc); > + argc = make_argv((char *)opts, argv, argc); > + > /* TODO: Restrict the forked process' permissions and capabilities. */ > rpid = qemu_fork(&local_error); > > if (rpid == -1) { > error_setg(errp, "Unable to spawn emulation program."); > close(fd[0]); > - close(fd[1]); > - return -1; > + goto fail; > } > > if (rpid == 0) { > close(fd[0]); > > - args[0] = g_strdup(exec_name); > - args[1] = g_strdup_printf("%d", fd[1]); > - args[2] = NULL; > - execvp(args[0], (char *const *)args); > + rc = execv(argv[0], (char *const *)argv); > exit(1); > } > pdev->remote_pid = rpid; > + pdev->socket = fd[0]; > + > + rc = 0; > > +fail: > close(fd[1]); > > - return 0; > + return rc; > } > > static int get_proxy_sock(PCIDevice *dev) > @@ -177,16 +214,17 @@ static void pci_proxy_dev_register_types(void) > type_init(pci_proxy_dev_register_types) > > static void init_proxy(PCIDevice *dev, char *command, char *exec_name, > - Error **errp) > + bool need_spawn, Error **errp) > { > PCIProxyDev *pdev = PCI_PROXY_DEV(dev); > Error *local_error = NULL; > > if (!pdev->managed) { > - if (command) { > - remote_spawn(pdev, command, exec_name, &local_error); > - } else { > - return; > + if (need_spawn) { > + if (remote_spawn(pdev, command, exec_name, &local_error)) { > + error_propagate(errp, local_error); > + return; > + } > } > } else { > pdev->remote_pid = atoi(pdev->rid); > diff --git a/include/hw/proxy/qemu-proxy.h b/include/hw/proxy/qemu-proxy.h > index 28b0114..29fa2e9 100644 > --- a/include/hw/proxy/qemu-proxy.h > +++ b/include/hw/proxy/qemu-proxy.h > @@ -39,7 +39,7 @@ typedef struct PCIProxyDev { > > void (*proxy_ready) (PCIDevice *dev); > void (*init_proxy) (PCIDevice *dev, char *command, char *exec_name, > - Error **errp); > + bool need_spawn, Error **errp); > > } PCIProxyDev; > >
On Mon, Mar 02, 2020 at 06:36:13PM +0100, Philippe Mathieu-Daudé wrote: > typo "multi" in patch subject. > > 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? g_shell_parse_argv() perhaps 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. Regards, Daniel
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? Thanks! Elena > > 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 :| >
* Jagannathan Raman (jag.raman@oracle.com) 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; ... > + > +static int make_argv(char *opts_str, char **argv, int argc) > +{ > + int max_args = 64; ..... > + > static int remote_spawn(PCIProxyDev *pdev, const char *opts, > const char *exec_name, Error **errp) > { > - char *args[3]; > pid_t rpid; > int fd[2] = {-1, -1}; > Error *local_error = NULL; > + char *argv[64]; Magic '64' in a lot of places; that should be one constant somewhere (if it's actually needed). Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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. 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
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. Few things we discussed internally is to have the remote drive command line options passed over by messages or using QMP. Thank you! Elena > 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 :| >
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. > Few things we discussed internally is to have the remote drive > command line options passed over by messages or using QMP. Regards, Daniel
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? Thank you! Elena Stefan, > > > Few things we discussed internally is to have the remote drive > > command line options passed over by messages or using QMP. > > 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 :| >
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
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 <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. 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
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; + + return argc; +} + static int remote_spawn(PCIProxyDev *pdev, const char *opts, const char *exec_name, Error **errp) { - char *args[3]; pid_t rpid; int fd[2] = {-1, -1}; Error *local_error = NULL; + char *argv[64]; + int argc = 0; + char *sfd; + char *exec_dir; int rc = -EINVAL; if (pdev->managed) { /* Child is forked by external program (such as libvirt). */ error_setg(errp, "Remote processed is managed and launched by external program"); - return -1; + return rc; } if (!exec_name) { @@ -41,32 +72,38 @@ static int remote_spawn(PCIProxyDev *pdev, const char *opts, if (socketpair(AF_UNIX, SOCK_STREAM, 0, fd)) { error_setg(errp, "Unable to create unix socket."); - return -1; + return rc; } + exec_dir = g_strdup_printf("%s/%s", qemu_get_exec_dir(), exec_name); + argc = add_argv(exec_dir, argv, argc); + sfd = g_strdup_printf("%d", fd[1]); + argc = add_argv(sfd, argv, argc); + argc = make_argv((char *)opts, argv, argc); + /* TODO: Restrict the forked process' permissions and capabilities. */ rpid = qemu_fork(&local_error); if (rpid == -1) { error_setg(errp, "Unable to spawn emulation program."); close(fd[0]); - close(fd[1]); - return -1; + goto fail; } if (rpid == 0) { close(fd[0]); - args[0] = g_strdup(exec_name); - args[1] = g_strdup_printf("%d", fd[1]); - args[2] = NULL; - execvp(args[0], (char *const *)args); + rc = execv(argv[0], (char *const *)argv); exit(1); } pdev->remote_pid = rpid; + pdev->socket = fd[0]; + + rc = 0; +fail: close(fd[1]); - return 0; + return rc; } static int get_proxy_sock(PCIDevice *dev) @@ -177,16 +214,17 @@ static void pci_proxy_dev_register_types(void) type_init(pci_proxy_dev_register_types) static void init_proxy(PCIDevice *dev, char *command, char *exec_name, - Error **errp) + bool need_spawn, Error **errp) { PCIProxyDev *pdev = PCI_PROXY_DEV(dev); Error *local_error = NULL; if (!pdev->managed) { - if (command) { - remote_spawn(pdev, command, exec_name, &local_error); - } else { - return; + if (need_spawn) { + if (remote_spawn(pdev, command, exec_name, &local_error)) { + error_propagate(errp, local_error); + return; + } } } else { pdev->remote_pid = atoi(pdev->rid); diff --git a/include/hw/proxy/qemu-proxy.h b/include/hw/proxy/qemu-proxy.h index 28b0114..29fa2e9 100644 --- a/include/hw/proxy/qemu-proxy.h +++ b/include/hw/proxy/qemu-proxy.h @@ -39,7 +39,7 @@ typedef struct PCIProxyDev { void (*proxy_ready) (PCIDevice *dev); void (*init_proxy) (PCIDevice *dev, char *command, char *exec_name, - Error **errp); + bool need_spawn, Error **errp); } PCIProxyDev;