| Message ID | 8d1ffc03424e664000eb65186bef0362cd1a5fd6.1571905346.git.jag.raman@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initial support of multi-process qemu | expand |
On Thu, Oct 24, 2019 at 05:08:55AM -0400, 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> > --- > New patch in v3 > > hw/proxy/qemu-proxy.c | 80 +++++++++++++++++++++++++++++++++---------- > include/hw/proxy/qemu-proxy.h | 2 +- > 2 files changed, 62 insertions(+), 20 deletions(-) > > diff --git a/hw/proxy/qemu-proxy.c b/hw/proxy/qemu-proxy.c > index baba4da..ca7dd1a 100644 > --- a/hw/proxy/qemu-proxy.c > +++ b/hw/proxy/qemu-proxy.c > @@ -45,47 +45,89 @@ > > static void pci_proxy_dev_realize(PCIDevice *dev, Error **errp); > > +static int add_argv(char *command_str, char **argv, int argc) > +{ > + int max_args = 64; > + > + if (argc < max_args - 1) { > + argv[argc++] = command_str; > + argv[argc] = 0; > + } else { > + return 0; > + } > + > + return argc; > +} > + > +static int make_argv(char *command_str, char **argv, int argc) > +{ > + int max_args = 64; > + > + char *p2 = strtok(command_str, " "); > + while (p2 && argc < max_args - 1) { > + argv[argc++] = p2; > + p2 = strtok(0, " "); > + } > + argv[argc] = 0; > + > + return argc; > +} So "command" isn't really the command-line, it's a string of options to append to the hardcoded qemu-scsi-dev command? This needs to command-line string construction needs to be cleaned up and the hardcoded qemu-scsi-dev needs to be replaced with an argument. > + > int remote_spawn(PCIProxyDev *pdev, const char *command, Error **errp) Error handling code is currently inconsistent because there is an int -errno return value and an errp argument. For example, errp isn't set when pdev->managed == true. The int -errno return value isn't needed. It can be just be bool and errp should be set in every single error code path. > { > - char *args[3]; > pid_t rpid; > int fd[2] = {-1, -1}; > Error *local_error = NULL; > + char *argv[64]; > + int argc = 0, _argc; > + char *sfd; > + char *exec_dir; > + int rc = -EINVAL; > > if (pdev->managed) { > /* Child is forked by external program (such as libvirt). */ > - return -1; > + return rc; > } > > 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(), "qemu-scsi-dev"); > + argc = add_argv(exec_dir, argv, argc); > + sfd = g_strdup_printf("%d", fd[1]); > + argc = add_argv(sfd, argv, argc); > + _argc = argc; > + argc = make_argv((char *)command, 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(command); > - args[1] = g_strdup_printf("%d", fd[1]); > - args[2] = NULL; > - execvp(args[0], (char *const *)args); > + execvp(argv[0], (char *const *)argv); > exit(1); > } > pdev->remote_pid = rpid; > - pdev->rsocket = fd[0]; > + pdev->rsocket = fd[1]; > + pdev->socket = fd[0]; Please choose meaningful names for these fields. I'm not sure why both need to be kept around though... > > + rc = 0; > + > +fail: > close(fd[1]); > > - return 0; > + for (int i = 0; i < _argc; i++) { > + g_free(argv[i]); > + } > + > + return rc; > } > > static int get_proxy_sock(PCIDevice *dev) > @@ -94,7 +136,7 @@ static int get_proxy_sock(PCIDevice *dev) > > pdev = PCI_PROXY_DEV(dev); > > - return pdev->rsocket; > + return pdev->socket; > } > > static void set_proxy_sock(PCIDevice *dev, int socket) > @@ -103,7 +145,7 @@ static void set_proxy_sock(PCIDevice *dev, int socket) > > pdev = PCI_PROXY_DEV(dev); > > - pdev->rsocket = socket; > + pdev->socket = socket; > pdev->managed = true; > > } > @@ -198,16 +240,16 @@ static void pci_proxy_dev_register_types(void) > > type_init(pci_proxy_dev_register_types) > > -static void init_proxy(PCIDevice *dev, char *command, Error **errp) > +static void init_proxy(PCIDevice *dev, char *command, bool need_spawn, Error **errp) > { > PCIProxyDev *pdev = PCI_PROXY_DEV(dev); > Error *local_error = NULL; > > if (!pdev->managed) { > - if (command) { > - remote_spawn(pdev, command, &local_error); > - } else { > - return; > + if (need_spawn) { pdev->managed, command == NULL, need_spawn are all ways of checking whether the remote process needs to be spawned. Why are all of them necessary and can they be simplified? > + if (!remote_spawn(pdev, command, &local_error)) { > + return; local_error needs to be propagated.
diff --git a/hw/proxy/qemu-proxy.c b/hw/proxy/qemu-proxy.c index baba4da..ca7dd1a 100644 --- a/hw/proxy/qemu-proxy.c +++ b/hw/proxy/qemu-proxy.c @@ -45,47 +45,89 @@ static void pci_proxy_dev_realize(PCIDevice *dev, Error **errp); +static int add_argv(char *command_str, char **argv, int argc) +{ + int max_args = 64; + + if (argc < max_args - 1) { + argv[argc++] = command_str; + argv[argc] = 0; + } else { + return 0; + } + + return argc; +} + +static int make_argv(char *command_str, char **argv, int argc) +{ + int max_args = 64; + + char *p2 = strtok(command_str, " "); + while (p2 && argc < max_args - 1) { + argv[argc++] = p2; + p2 = strtok(0, " "); + } + argv[argc] = 0; + + return argc; +} + int remote_spawn(PCIProxyDev *pdev, const char *command, Error **errp) { - char *args[3]; pid_t rpid; int fd[2] = {-1, -1}; Error *local_error = NULL; + char *argv[64]; + int argc = 0, _argc; + char *sfd; + char *exec_dir; + int rc = -EINVAL; if (pdev->managed) { /* Child is forked by external program (such as libvirt). */ - return -1; + return rc; } 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(), "qemu-scsi-dev"); + argc = add_argv(exec_dir, argv, argc); + sfd = g_strdup_printf("%d", fd[1]); + argc = add_argv(sfd, argv, argc); + _argc = argc; + argc = make_argv((char *)command, 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(command); - args[1] = g_strdup_printf("%d", fd[1]); - args[2] = NULL; - execvp(args[0], (char *const *)args); + execvp(argv[0], (char *const *)argv); exit(1); } pdev->remote_pid = rpid; - pdev->rsocket = fd[0]; + pdev->rsocket = fd[1]; + pdev->socket = fd[0]; + rc = 0; + +fail: close(fd[1]); - return 0; + for (int i = 0; i < _argc; i++) { + g_free(argv[i]); + } + + return rc; } static int get_proxy_sock(PCIDevice *dev) @@ -94,7 +136,7 @@ static int get_proxy_sock(PCIDevice *dev) pdev = PCI_PROXY_DEV(dev); - return pdev->rsocket; + return pdev->socket; } static void set_proxy_sock(PCIDevice *dev, int socket) @@ -103,7 +145,7 @@ static void set_proxy_sock(PCIDevice *dev, int socket) pdev = PCI_PROXY_DEV(dev); - pdev->rsocket = socket; + pdev->socket = socket; pdev->managed = true; } @@ -198,16 +240,16 @@ static void pci_proxy_dev_register_types(void) type_init(pci_proxy_dev_register_types) -static void init_proxy(PCIDevice *dev, char *command, Error **errp) +static void init_proxy(PCIDevice *dev, char *command, bool need_spawn, Error **errp) { PCIProxyDev *pdev = PCI_PROXY_DEV(dev); Error *local_error = NULL; if (!pdev->managed) { - if (command) { - remote_spawn(pdev, command, &local_error); - } else { - return; + if (need_spawn) { + if (!remote_spawn(pdev, command, &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 3648a77..f97b103 100644 --- a/include/hw/proxy/qemu-proxy.h +++ b/include/hw/proxy/qemu-proxy.h @@ -63,7 +63,7 @@ typedef struct PCIProxyDev { void (*set_remote_opts) (PCIDevice *dev, QDict *qdict, unsigned int cmd); void (*proxy_ready) (PCIDevice *dev); - void (*init_proxy) (PCIDevice *pdev, char *command, Error **errp); + void (*init_proxy) (PCIDevice *dev, char *command, bool need_spawn, Error **errp); } PCIProxyDev;