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

Re: [libvirt] [PATCH 15/27] Add API for issugin 'migrate' command with exec protocol



Subject typo - "issugin"

On Thu, 2009-09-24 at 16:00 +0100, Daniel P. Berrange wrote:
> * src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h: Add new
>   qemuMonitorMigrateToCommand() API
> * src/qemu/qemu_driver.c: Switch over to using the
>   qemuMonitorMigrateToCommand() API for core dumps and save
>   to file APIs
> ---
>  src/libvirt_private.syms     |    1 +
>  src/qemu/qemu_driver.c       |  119 ++++++++----------------------------------
>  src/qemu/qemu_monitor_text.c |   63 +++++++++++++++++++++--
>  src/qemu/qemu_monitor_text.h |    4 ++
>  4 files changed, 86 insertions(+), 101 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index a6668f3..f8598c7 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -453,6 +453,7 @@ virGetGroupID;
>  virFileFindMountPoint;
>  virFileWaitForDevices;
>  virFileMatchesNameSuffix;
> +virArgvToString;
>  
> 
>  # usb.h
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f234639..da08af9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -3170,11 +3170,6 @@ static char *qemudEscapeMonitorArg(const char *in)
>      return qemudEscape(in, 0);
>  }
>  
> -static char *qemudEscapeShellArg(const char *in)
> -{
> -    return qemudEscape(in, 1);
> -}
> -
>  #define QEMUD_SAVE_MAGIC "LibvirtQemudSave"
>  #define QEMUD_SAVE_VERSION 2
>  
> @@ -3217,15 +3212,11 @@ static int qemudDomainSave(virDomainPtr dom,
>  {
>      struct qemud_driver *driver = dom->conn->privateData;
>      virDomainObjPtr vm = NULL;
> -    char *command = NULL;
> -    char *info = NULL;
>      int fd = -1;
> -    char *safe_path = NULL;
>      char *xml = NULL;
>      struct qemud_save_header header;
>      int ret = -1;
>      virDomainEventPtr event = NULL;
> -    int internalret;
>  
>      memset(&header, 0, sizeof(header));
>      memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic));
> @@ -3267,7 +3258,6 @@ static int qemudDomainSave(virDomainPtr dom,
>          if (qemuMonitorStopCPUs(vm) < 0)
>              goto cleanup;
>          vm->state = VIR_DOMAIN_PAUSED;
> -        VIR_FREE(info);
>      }
>  
>      /* Get XML for the domain */
> @@ -3306,55 +3296,21 @@ static int qemudDomainSave(virDomainPtr dom,
>      }
>      fd = -1;
>  
> -    /* Migrate to file */
> -    safe_path = qemudEscapeShellArg(path);
> -    if (!safe_path) {
> -        virReportOOMError(dom->conn);
> -        goto cleanup;
> -    }
> -
> -    {
> +    if (header.compressed == QEMUD_SAVE_FORMAT_RAW) {
> +        const char *args[] = { "cat", NULL };
> +        ret = qemuMonitorMigrateToCommand(vm, args, path);
> +    } else {
>          const char *prog = qemudSaveCompressionTypeToString(header.compressed);
> -        const char *args;
> -
> -        if (prog == NULL) {
> -            qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
> -                             _("Invalid compress format %d"), header.compressed);
> -            goto cleanup;
> -        }
> -
> -        if (STREQ (prog, "raw")) {
> -            prog = "cat";
> -            args = "";
> -        } else {
> -            args = "-c";
> -        }

Using the enum value to catch the raw case rather than the "raw" string
is a sensible change

> -        internalret = virAsprintf(&command, "migrate \"exec:"
> -                                  "%s %s >> '%s' 2>/dev/null\"", prog, args,
> -                                  safe_path);
> -    }
> -
> -    if (internalret < 0) {
> -        virReportOOMError(dom->conn);
> -        goto cleanup;
> -    }
> -
> -    if (qemudMonitorCommand(vm, command, &info) < 0) {
> -        qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
> -                         "%s", _("migrate operation failed"));
> -        goto cleanup;
> +        const char *args[] = {
> +            prog,
> +            "-c",
> +            NULL
> +        };
> +        ret = qemuMonitorMigrateToCommand(vm, args, path);
>      }
>  
> -    DEBUG ("%s: migrate reply: %s", vm->def->name, info);
> -
> -    /* If the command isn't supported then qemu prints:
> -     * unknown command: migrate" */
> -    if (strstr(info, "unknown command:")) {
> -        qemudReportError (dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
> -                          "%s",
> -                          _("'migrate' not supported by this qemu"));
> +    if (ret < 0)
>          goto cleanup;
> -    }
>  
>      /* Shut it down */
>      qemudShutdownVMDaemon(dom->conn, driver, vm);
> @@ -3366,15 +3322,11 @@ static int qemudDomainSave(virDomainPtr dom,
>                                  vm);
>          vm = NULL;
>      }
> -    ret = 0;
>  
>  cleanup:
>      if (fd != -1)
>          close(fd);
>      VIR_FREE(xml);
> -    VIR_FREE(safe_path);
> -    VIR_FREE(command);
> -    VIR_FREE(info);
>      if (ret != 0)
>          unlink(path);
>      if (vm)
> @@ -3391,11 +3343,12 @@ static int qemudDomainCoreDump(virDomainPtr dom,
>                                 int flags ATTRIBUTE_UNUSED) {
>      struct qemud_driver *driver = dom->conn->privateData;
>      virDomainObjPtr vm;
> -    char *command = NULL;
> -    char *info = NULL;
> -    char *safe_path = NULL;
>      int resume = 0, paused = 0;
>      int ret = -1;
> +    const char *args[] = {
> +        "cat",
> +        NULL,
> +    };
>  
>      qemuDriverLock(driver);
>      vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> @@ -3427,43 +3380,9 @@ static int qemudDomainCoreDump(virDomainPtr dom,
>          paused = 1;
>      }
>  
> -    /* Migrate to file */
> -    safe_path = qemudEscapeShellArg(path);
> -    if (!safe_path) {
> -        virReportOOMError(dom->conn);
> -        goto cleanup;
> -    }
> -    if (virAsprintf(&command, "migrate \"exec:"
> -                  "dd of='%s' 2>/dev/null"
> -                  "\"", safe_path) == -1) {

Would have like to see switch from dd to cat as a separate patch with an
explanation in the changelog

> -        virReportOOMError(dom->conn);
> -        command = NULL;
> -        goto cleanup;
> -    }
> -
> -    if (qemudMonitorCommand(vm, command, &info) < 0) {
> -        qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
> -                         "%s", _("migrate operation failed"));
> -        goto cleanup;
> -    }
> -
> -    DEBUG ("%s: migrate reply: %s", vm->def->name, info);
> -
> -    /* If the command isn't supported then qemu prints:
> -     * unknown command: migrate" */
> -    if (strstr(info, "unknown command:")) {
> -        qemudReportError (dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
> -                          "%s",
> -                          _("'migrate' not supported by this qemu"));
> -        goto cleanup;
> -    }
> -
> +    ret = qemuMonitorMigrateToCommand(vm, args, path);
>      paused = 1;
> -    ret = 0;
>  cleanup:
> -    VIR_FREE(safe_path);
> -    VIR_FREE(command);
> -    VIR_FREE(info);
>  
>      /* Since the monitor is always attached to a pty for libvirt, it
>         will support synchronous operations so we always get here after
> @@ -6370,6 +6289,11 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn,
>              goto cleanup;
>          }
>  
> +        /* XXX this really should have been a properly well-formed
> +         * URI, but we can't add in tcp:// now without breaking
> +         * compatability with old targets. We at least make the
> +         * new targets accept both syntaxes though.
> +         */
>          /* Caller frees */
>          internalret = virAsprintf(uri_out, "tcp:%s:%d", hostname, this_port);
>          VIR_FREE(hostname);
> @@ -6536,6 +6460,7 @@ qemudDomainMigratePerform (virDomainPtr dom,
>  
>      /* Issue the migrate command. */
>      if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) {
> +        /* HACK: source host generates bogus URIs, so fix them up */
>          char *tmpuri;
>          if (virAsprintf(&tmpuri, "tcp://%s", uri + strlen("tcp:")) < 0) {
>              virReportOOMError(dom->conn);

Two unrelated comments - looks like it's more related to the URI changes
you made in the previous patch

> diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> index 4f8d72e..c154019 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -118,6 +118,10 @@ static char *qemudEscapeMonitorArg(const char *in)
>      return qemudEscape(in, 0);
>  }
>  
> +static char *qemudEscapeShellArg(const char *in)
> +{
> +    return qemudEscape(in, 1);
> +}
>  
>  /* Throw away any data available on the monitor
>   * This is done before executing a command, in order
> @@ -1096,12 +1100,18 @@ static int qemuMonitorMigrate(const virDomainObjPtr vm,
>      char *cmd = NULL;
>      char *info = NULL;
>      int ret = -1;
> +    char *safedest = qemudEscapeMonitorArg(dest);
>  
> -    if (virAsprintf(&cmd, "migrate %s", cmd) < 0) {
> +    if (!safedest) {
>          virReportOOMError(NULL);
>          return -1;
>      }
>  
> +    if (virAsprintf(&cmd, "migrate \"%s\"", safedest) < 0) {
> +        virReportOOMError(NULL);
> +        goto cleanup;
> +    }

Little bit worried that the introduction of escaping will break other
usages, but it should be fine

> +
>      if (qemudMonitorCommand(vm, cmd, &info) < 0) {
>          qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
>                           _("unable to start migration to %s"), dest);
> @@ -1112,8 +1122,15 @@ static int qemuMonitorMigrate(const virDomainObjPtr vm,
>  
>      /* Now check for "fail" in the output string */
>      if (strstr(info, "fail") != NULL) {
> -        qemudReportError (NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED,
> -                          _("migration to '%s' failed: %s"), dest, info);
> +        qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED,
> +                         _("migration to '%s' failed: %s"), dest, info);

Unrelated

> +        goto cleanup;
> +    }
> +    /* If the command isn't supported then qemu prints:
> +     * unknown command: migrate" */
> +    if (strstr(info, "unknown command:")) {
> +        qemudReportError(NULL, NULL, NULL, VIR_ERR_NO_SUPPORT,
> +                         _("migration to '%s' not supported by this qemu: %s"), dest, info);
>          goto cleanup;
>      }
>  
> @@ -1121,6 +1138,7 @@ static int qemuMonitorMigrate(const virDomainObjPtr vm,
>      ret = 0;
>  
>  cleanup:
> +    VIR_FREE(safedest);
>      VIR_FREE(info);
>      VIR_FREE(cmd);
>      return ret;
> @@ -1130,7 +1148,7 @@ int qemuMonitorMigrateToHost(const virDomainObjPtr vm,
>                               const char *hostname,
>                               int port)
>  {
> -    char *uri;
> +    char *uri = NULL;

Unrelated

ACK

Cheers,
Mark.


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