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

Re: [libvirt] [PATCH 2/4] qemu: add support for dump-guest-core option



On 15.08.2012 11:25, Martin Kletzander wrote:
> The "dump-guest-core' option is new option for the machine type
> (-machine pc,dump-guest-core) that controls whether the guest memory
> will be marked as dumpable.
> 
> While testing this, I've found out that the value for the '-M' options
> is not parsed correctly when additional parameters are used. However,
> when '-machine' is used for the same options, it gets parsed as
> expected. That's why this patch also modifies the parsing and creating
> of the command line, so both '-M' and '-machine' are recognized. In
> QEMU's help there is only mention of the 'machine parameter now with
> no sign of the older '-M'.
> ---
>  src/qemu/qemu_capabilities.c |    4 ++
>  src/qemu/qemu_capabilities.h |    1 +
>  src/qemu/qemu_command.c      |   89 +++++++++++++++++++++++++++++++++++++----
>  3 files changed, 85 insertions(+), 9 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index b8160b6..37d96b3 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -175,6 +175,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
>                "disable-s3",
>                "disable-s4",
> 
> +              "dump-guest-core", /* 101 */
>      );
> 
>  struct qemu_feature_flags {
> @@ -1175,6 +1176,9 @@ qemuCapsComputeCmdFlags(const char *help,
>      if (strstr(help, "-no-shutdown") && (version < 14000 || version > 15000))
>          qemuCapsSet(flags, QEMU_CAPS_NO_SHUTDOWN);
> 
> +    if (strstr(help, "dump-guest-core=on|off"))
> +        qemuCapsSet(flags, QEMU_CAPS_DUMP_GUEST_CORE);
> +
>      /*
>       * Handling of -incoming arg with varying features
>       *  -incoming tcp    (kvm >= 79, qemu >= 0.10.0)
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index e49424a..f35b1b5 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -140,6 +140,7 @@ enum qemuCapsFlags {
>      QEMU_CAPS_VIRTIO_SCSI_PCI    = 102, /* -device virtio-scsi-pci */
>      QEMU_CAPS_DISABLE_S3         = 103, /* S3 BIOS Advertisement on/off */
>      QEMU_CAPS_DISABLE_S4         = 104, /* S4 BIOS Advertisement on/off */
> +    QEMU_CAPS_DUMP_GUEST_CORE    = 105, /* dump-guest-core parameter */
> 
>      QEMU_CAPS_LAST,                   /* this must always be the last item */
>  };
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 34ee00e..0af5233 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4190,6 +4190,53 @@ no_memory:
>      goto cleanup;
>  }
> 
> +static int
> +qemuBuildMachineArgStr(virCommandPtr cmd,
> +                       const virDomainDefPtr def,
> +                       virBitmapPtr qemuCaps)
> +{
> +    /* This should *never* be NULL, since we always provide
> +     * a machine in the capabilities data for QEMU. So this
> +     * check is just here as a safety in case the unexpected
> +     * happens */
> +    if (def->os.machine) {

If you'd rewrite this:

  if (!def->os.machine)
      return 0;

then you can move this code one level of nesting higher.
> +        if (!def->mem.dump_core) {
> +            /* if no parameter to the machine type is needed, we still use
> +             * '-M' to keep the most of the compatibility with older versions.
> +             */
> +            virCommandAddArgList(cmd, "-M", def->os.machine, NULL);
> +        } else {
> +            char *machine = NULL;
> +
> +            if (!qemuCapsGet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               "%s", _("dump-guest-core is not available "
> +                                       " with this QEMU binary"));
> +                return -1;
> +            }
> +
> +            virAsprintf(&machine,
> +                        "%s,%s=%s",
> +                        def->os.machine,
> +                        "dump-guest-core",
> +                        virDomainMemDumpTypeToString(def->mem.dump_core));
> +
> +            if (machine == NULL) {
> +                virReportOOMError();
> +                return -1;
> +            }
> +
> +            /* However, in case there is a parameter to be added, we need to
> +             * use the "-machine" parameter because qemu is not parsing the
> +             * "-M" correctly */
> +            virCommandAddArgList(cmd, "-machine", machine, NULL);
> +            VIR_FREE(machine);
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static char *
>  qemuBuildSmpArgStr(const virDomainDefPtr def,
>                     virBitmapPtr qemuCaps)
> @@ -4403,12 +4450,8 @@ qemuBuildCommandLine(virConnectPtr conn,
>      }
>      virCommandAddArg(cmd, "-S"); /* freeze CPU */
> 
> -    /* This should *never* be NULL, since we always provide
> -     * a machine in the capabilities data for QEMU. So this
> -     * check is just here as a safety in case the unexpected
> -     * happens */
> -    if (def->os.machine)
> -        virCommandAddArgList(cmd, "-M", def->os.machine, NULL);
> +    if (qemuBuildMachineArgStr(cmd, def, qemuCaps) < 0)
> +        goto error;
> 
>      if (qemuBuildCpuArgStr(driver, def, emulator, qemuCaps,
>                             &ut, &cpu, &hasHwVirt, !!migrateFrom) < 0)
> @@ -8091,10 +8134,38 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
>              }
>              if (STREQ(def->name, ""))
>                  VIR_FREE(def->name);
> -        } else if (STREQ(arg, "-M")) {
> +        } else if (STREQ(arg, "-M") ||
> +                   STREQ(arg, "-machine")) {
> +            char *params;
>              WANT_VALUE();
> -            if (!(def->os.machine = strdup(val)))
> -                goto no_memory;
> +            params = strchr(val, ',');
> +            if (params == NULL) {
> +                if (!(def->os.machine = strdup(val)))
> +                    goto no_memory;
> +            } else {
> +                if (!(def->os.machine = strndup(val, params - val)))
> +                    goto no_memory;
> +
> +                while(params++) {
> +                    /* prepared for more "-machine" parameters */
> +                    const char *tmp = params;

This looks rather odd ... [1]

> +                    params = strchr(params, ',');
> +
> +                    if (STRPREFIX(tmp, "dump-guest-core=")) {
> +                        tmp += strlen("dump-guest-core=");
> +                        if (params) {
> +                            tmp = strndup(tmp, params - tmp);

[1] ... so does this ...

> +                            if (tmp == NULL)
> +                                goto no_memory;
> +                        }
> +                        def->mem.dump_core = virDomainMemDumpTypeFromString(tmp);
> +                        if (def->mem.dump_core == -1)

s/== -1/< 0/

> +                            def->mem.dump_core = VIR_DOMAIN_MEM_DUMP_DEFAULT;
> +                        if (params)
> +                            VIR_FREE(tmp);

[1] ... and this. Drop the const keyword.

> +                    }
> +                }
> +            }
>          } else if (STREQ(arg, "-serial")) {
>              WANT_VALUE();
>              if (STRNEQ(val, "none")) {
> 

ACK with those nits fixed.

Michal


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