Re: [PATCH v2 01/31] qemu: Make QMP probing process reusable

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Nov 21, 2016 at 12:20:57AM +0100, Jiri Denemark wrote:
> The code that runs a new QEMU process to be used for probing
> capabilities is separated into four reusable functions so that any code
> that wants to probe a QEMU process may just follow a few simple steps:
> 
>     cmd = virQEMUCapsInitQMPCommandNew(...);
> 
>     mon = virQEMUCapsInitQMPCommandRun(cmd, ...);
> 
>     /* talk to the running QEMU process using its QMP monitor */
> 
>     if (reprobeIsRequired) {
>         virQEMUCapsInitQMPCommandAbort(cmd, ...);
>         mon = virQEMUCapsInitQMPCommandRun(cmd, ...);
> 
>         /* talk to the running QEMU process again */
>     }
> 
>     virQEMUCapsInitQMPCommandFree(cmd);
> 
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
>  src/qemu/qemu_capabilities.c | 259 +++++++++++++++++++++++++++++--------------
>  1 file changed, 174 insertions(+), 85 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index cfd090c..5ae57be 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4022,32 +4022,101 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
>      return ret;
>  }
>  
> -static int
> -virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> -                   const char *libDir,
> -                   uid_t runUid,
> -                   gid_t runGid,
> -                   char **qmperr)
> -{
> -    int ret = -1;
> -    virCommandPtr cmd = NULL;
> -    qemuMonitorPtr mon = NULL;
> -    int status = 0;
> +
> +typedef struct _virQEMUCapsInitQMPCommand virQEMUCapsInitQMPCommand;
> +typedef virQEMUCapsInitQMPCommand *virQEMUCapsInitQMPCommandPtr;
> +struct _virQEMUCapsInitQMPCommand {
> +    char *binary;
> +    uid_t runUid;
> +    gid_t runGid;
> +    char **qmperr;
> +    char *monarg;
> +    char *monpath;
> +    char *pidfile;
> +    virCommandPtr cmd;
> +    qemuMonitorPtr mon;
>      virDomainChrSourceDef config;
> -    char *monarg = NULL;
> -    char *monpath = NULL;
> -    char *pidfile = NULL;
> -    pid_t pid = 0;
> -    virDomainObjPtr vm = NULL;
> -    virDomainXMLOptionPtr xmlopt = NULL;
> +    pid_t pid;
> +    virDomainObjPtr vm;
> +};
> +
> +
> +static void
> +virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd)
> +{
> +    if (cmd->mon)
> +        virObjectUnlock(cmd->mon);
> +    qemuMonitorClose(cmd->mon);
> +    cmd->mon = NULL;
> +
> +    virCommandAbort(cmd->cmd);
> +    virCommandFree(cmd->cmd);
> +    cmd->cmd = NULL;
> +
> +    if (cmd->monpath)
> +        ignore_value(unlink(cmd->monpath));
> +
> +    virDomainObjEndAPI(&cmd->vm);
> +
> +    if (cmd->pid != 0) {
> +        char ebuf[1024];
> +
> +        VIR_DEBUG("Killing QMP caps process %lld", (long long) cmd->pid);
> +        if (virProcessKill(cmd->pid, SIGKILL) < 0 && errno != ESRCH)
> +            VIR_ERROR(_("Failed to kill process %lld: %s"),
> +                      (long long) cmd->pid,
> +                      virStrerror(errno, ebuf, sizeof(ebuf)));
> +
> +        VIR_FREE(*cmd->qmperr);
> +    }
> +    if (cmd->pidfile)
> +        unlink(cmd->pidfile);
> +    cmd->pid = 0;
> +}
> +
> +
> +static void
> +virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd)
> +{
> +    if (!cmd)
> +        return;
> +
> +    virQEMUCapsInitQMPCommandAbort(cmd);
> +    VIR_FREE(cmd->binary);
> +    VIR_FREE(cmd->monpath);
> +    VIR_FREE(cmd->monarg);
> +    VIR_FREE(cmd->pidfile);
> +    VIR_FREE(cmd);
> +}
> +
> +
> +static virQEMUCapsInitQMPCommandPtr
> +virQEMUCapsInitQMPCommandNew(char *binary,
> +                             const char *libDir,
> +                             uid_t runUid,
> +                             gid_t runGid,
> +                             char **qmperr)
> +{
> +    virQEMUCapsInitQMPCommandPtr cmd = NULL;
> +
> +    if (VIR_ALLOC(cmd) < 0)
> +        goto error;
> +
> +    if (VIR_STRDUP(cmd->binary, binary) < 0)
> +        goto error;
> +
> +    cmd->runUid = runUid;
> +    cmd->runGid = runGid;
> +    cmd->qmperr = qmperr;
>  
>      /* the ".sock" sufix is important to avoid a possible clash with a qemu
>       * domain called "capabilities"
>       */
> -    if (virAsprintf(&monpath, "%s/%s", libDir, "capabilities.monitor.sock") < 0)
> -        goto cleanup;
> -    if (virAsprintf(&monarg, "unix:%s,server,nowait", monpath) < 0)
> -        goto cleanup;
> +    if (virAsprintf(&cmd->monpath, "%s/%s", libDir,
> +                    "capabilities.monitor.sock") < 0)
> +        goto error;
> +    if (virAsprintf(&cmd->monarg, "unix:%s,server,nowait", cmd->monpath) < 0)
> +        goto error;
>  
>      /* ".pidfile" suffix is used rather than ".pid" to avoid a possible clash
>       * with a qemu domain called "capabilities"
> @@ -4055,17 +4124,31 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>       * -daemonize we need QEMU to be allowed to create them, rather
>       * than libvirtd. So we're using libDir which QEMU can write to
>       */
> -    if (virAsprintf(&pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0)
> -        goto cleanup;
> +    if (virAsprintf(&cmd->pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0)
> +        goto error;
>  
> -    memset(&config, 0, sizeof(config));
> -    config.type = VIR_DOMAIN_CHR_TYPE_UNIX;
> -    config.data.nix.path = monpath;
> -    config.data.nix.listen = false;
> +    virPidFileForceCleanupPath(cmd->pidfile);
>  
> -    virPidFileForceCleanupPath(pidfile);
> +    cmd->config.type = VIR_DOMAIN_CHR_TYPE_UNIX;
> +    cmd->config.data.nix.path = cmd->monpath;
> +    cmd->config.data.nix.listen = false;
>  
> -    VIR_DEBUG("Try to get caps via QMP qemuCaps=%p", qemuCaps);
> +    return cmd;
> +
> + error:
> +    virQEMUCapsInitQMPCommandFree(cmd);
> +    return NULL;
> +}
> +
> +
> +static qemuMonitorPtr
> +virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd,
> +                             bool *qemuFailed)

That *bool* is not necessary, function virQEMUCapsInitQMPCommandRun can return
simple *int* because qemuMonitorPtr is stored in *cmd*.  So this function can
return -1 in case of fatal error.  In case return is 0, following code can
easily depend on whether cmd->mon is set or not.

Otherwise this patch looks good, but I think that it would be better to send a
new version with the suggested change.

Pavel

> +{
> +    virDomainXMLOptionPtr xmlopt = NULL;
> +    int status = 0;
> +
> +    VIR_DEBUG("Try to probe capabilities of '%s' via QMP", cmd->binary);
>  
>      /*
>       * We explicitly need to use -daemonize here, rather than
> @@ -4074,86 +4157,92 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>       * daemonize guarantees control won't return to libvirt
>       * until the socket is present.
>       */
> -    cmd = virCommandNewArgList(qemuCaps->binary,
> -                               "-S",
> -                               "-no-user-config",
> -                               "-nodefaults",
> -                               "-nographic",
> -                               "-M", "none",
> -                               "-qmp", monarg,
> -                               "-pidfile", pidfile,
> -                               "-daemonize",
> -                               NULL);
> -    virCommandAddEnvPassCommon(cmd);
> -    virCommandClearCaps(cmd);
> -    virCommandSetGID(cmd, runGid);
> -    virCommandSetUID(cmd, runUid);
> +    cmd->cmd = virCommandNewArgList(cmd->binary,
> +                                    "-S",
> +                                    "-no-user-config",
> +                                    "-nodefaults",
> +                                    "-nographic",
> +                                    "-M", "none",
> +                                    "-qmp", cmd->monarg,
> +                                    "-pidfile", cmd->pidfile,
> +                                    "-daemonize",
> +                                    NULL);
> +    virCommandAddEnvPassCommon(cmd->cmd);
> +    virCommandClearCaps(cmd->cmd);
> +    virCommandSetGID(cmd->cmd, cmd->runGid);
> +    virCommandSetUID(cmd->cmd, cmd->runUid);
>  
> -    virCommandSetErrorBuffer(cmd, qmperr);
> +    virCommandSetErrorBuffer(cmd->cmd, cmd->qmperr);
>  
>      /* Log, but otherwise ignore, non-zero status.  */
> -    if (virCommandRun(cmd, &status) < 0)
> +    if (virCommandRun(cmd->cmd, &status) < 0)
>          goto cleanup;
>  
>      if (status != 0) {
> -        ret = 0;
>          VIR_DEBUG("QEMU %s exited with status %d: %s",
> -                  qemuCaps->binary, status, *qmperr);
> -        goto cleanup;
> +                  cmd->binary, status, *cmd->qmperr);
> +        goto ignore;
>      }
>  
> -    if (virPidFileReadPath(pidfile, &pid) < 0) {
> -        VIR_DEBUG("Failed to read pidfile %s", pidfile);
> -        ret = 0;
> -        goto cleanup;
> +    if (virPidFileReadPath(cmd->pidfile, &cmd->pid) < 0) {
> +        VIR_DEBUG("Failed to read pidfile %s", cmd->pidfile);
> +        goto ignore;
>      }
>  
>      if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL)) ||
> -        !(vm = virDomainObjNew(xmlopt)))
> +        !(cmd->vm = virDomainObjNew(xmlopt)))
>          goto cleanup;
>  
> -    vm->pid = pid;
> +    cmd->vm->pid = cmd->pid;
>  
> -    if (!(mon = qemuMonitorOpen(vm, &config, true, &callbacks, NULL))) {
> -        ret = 0;
> +    if (!(cmd->mon = qemuMonitorOpen(cmd->vm, &cmd->config, true,
> +                                     &callbacks, NULL)))
> +        goto ignore;
> +
> +    virObjectLock(cmd->mon);
> +
> + cleanup:
> +    if (!cmd->mon)
> +        virQEMUCapsInitQMPCommandAbort(cmd);
> +    virObjectUnref(xmlopt);
> +
> +    return cmd->mon;
> +
> + ignore:
> +    *qemuFailed = true;
> +    goto cleanup;
> +}
> +
> +
> +static int
> +virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> +                   const char *libDir,
> +                   uid_t runUid,
> +                   gid_t runGid,
> +                   char **qmperr)
> +{
> +    virQEMUCapsInitQMPCommandPtr cmd = NULL;
> +    qemuMonitorPtr mon = NULL;
> +    bool qemuFailed = false;
> +    int ret = -1;
> +
> +    if (!(cmd = virQEMUCapsInitQMPCommandNew(qemuCaps->binary, libDir,
> +                                             runUid, runGid, qmperr)))
> +        goto cleanup;
> +
> +    if (!(mon = virQEMUCapsInitQMPCommandRun(cmd, &qemuFailed))) {
> +        if (qemuFailed)
> +            ret = 0;
>          goto cleanup;
>      }
>  
> -    virObjectLock(mon);
> -
>      if (virQEMUCapsInitQMPMonitor(qemuCaps, mon) < 0)
>          goto cleanup;
>  
>      ret = 0;
>  
>   cleanup:
> -    if (mon)
> -        virObjectUnlock(mon);
> -    qemuMonitorClose(mon);
> -    virCommandAbort(cmd);
> -    virCommandFree(cmd);
> -    VIR_FREE(monarg);
> -    if (monpath)
> -        ignore_value(unlink(monpath));
> -    VIR_FREE(monpath);
> -    virDomainObjEndAPI(&vm);
> -    virObjectUnref(xmlopt);
> -
> -    if (pid != 0) {
> -        char ebuf[1024];
> -
> -        VIR_DEBUG("Killing QMP caps process %lld", (long long) pid);
> -        if (virProcessKill(pid, SIGKILL) < 0 && errno != ESRCH)
> -            VIR_ERROR(_("Failed to kill process %lld: %s"),
> -                      (long long) pid,
> -                      virStrerror(errno, ebuf, sizeof(ebuf)));
> -
> -        VIR_FREE(*qmperr);
> -    }
> -    if (pidfile) {
> -        unlink(pidfile);
> -        VIR_FREE(pidfile);
> -    }
> +    virQEMUCapsInitQMPCommandFree(cmd);
>      return ret;
>  }
>  
> -- 
> 2.10.2
> 
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux