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

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



On 09/20/2012 02:54 PM, Michal Privoznik wrote:
> On 20.09.2012 10:58, 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      | 81 +++++++++++++++++++++++++++++++++++++++-----
>>  tests/qemuhelptest.c         |  3 +-
>>  4 files changed, 79 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index 3582cbd..4b52dc5 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -182,6 +182,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
>>                "seccomp-sandbox",
>>
>>                "reboot-timeout", /* 110 */
>> +              "dump-guest-core",
>>      );
>>
>>  struct _qemuCaps {
>> @@ -1243,6 +1244,9 @@ qemuCapsComputeCmdFlags(const char *help,
>>      if (strstr(help, "-no-shutdown") && (version < 14000 || version > 15000))
>>          qemuCapsSet(caps, QEMU_CAPS_NO_SHUTDOWN);
>>
>> +    if (strstr(help, "dump-guest-core=on|off"))
>> +        qemuCapsSet(caps, 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 2201cb3..a069f42 100644
>> --- a/src/qemu/qemu_capabilities.h
>> +++ b/src/qemu/qemu_capabilities.h
>> @@ -146,6 +146,7 @@ enum qemuCapsFlags {
>>      QEMU_CAPS_SCSI_DISK_WWN      = 108, /* Is scsi-disk.wwn available? */
>>      QEMU_CAPS_SECCOMP_SANDBOX    = 109, /* -sandbox */
>>      QEMU_CAPS_REBOOT_TIMEOUT     = 110, /* -boot reboot-timeout */
>> +    QEMU_CAPS_DUMP_GUEST_CORE    = 111, /* 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 3807596..52ad4d2 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -4306,6 +4306,45 @@ no_memory:
>>      goto cleanup;
>>  }
>>
>> +static int
>> +qemuBuildMachineArgStr(virCommandPtr cmd,
>> +                       const virDomainDefPtr def,
>> +                       qemuCapsPtr caps)
>> +{
>> +    /* 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)
>> +        return 0;
>> +
>> +    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);
> 
> I wonder if we should switch to -machine unconditionally on enough new
> qemu (option was introduced in 0.15) and leave -M just for backwards
> compatibility since qemu is (sort of) maintaining -M just because of us.
>  But that'd be a separate patch anyway.
> 

I asked about this on qemu-devel and we really should do that. I'm
planning on adding this (with '-machine' capabilities etc.), but as you
said, that is an issue for separate patch.

>> +    } else {
>> +        if (!qemuCapsGet(caps, QEMU_CAPS_DUMP_GUEST_CORE)) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                           "%s", _("dump-guest-core is not available "
>> +                                   " with this QEMU binary"));
>> +            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 */
>> +        virCommandAddArg(cmd, "-machine");
>> +        virCommandAddArgFormat(cmd,
>> +                               "%s,%s=%s",
>> +                               def->os.machine,
>> +                               "dump-guest-core",
>> +                               virDomainMemDumpTypeToString(def->mem.dump_core));
> 
> This looks weird to me:
> s/%s,%s=%s/%s,dump-guest-core=%s/
> 

Yep, dunno what I was thinking about in here. It works, though =)

>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static char *
>>  qemuBuildSmpArgStr(const virDomainDefPtr def,
>>                     qemuCapsPtr caps)
>> @@ -4498,12 +4537,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, caps) < 0)
>> +        goto error;
>>
>>      if (qemuBuildCpuArgStr(driver, def, emulator, caps,
>>                             &ut, &cpu, &hasHwVirt, !!migrateFrom) < 0)
>> @@ -8320,10 +8355,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 */
>> +                    char *tmp = params;
>> +                    params = strchr(params, ',');
>> +
>> +                    if (STRPREFIX(tmp, "dump-guest-core=")) {
>> +                        tmp += strlen("dump-guest-core=");
>> +                        if (params) {
>> +                            tmp = strndup(tmp, params - tmp);
>> +                            if (tmp == NULL)
>> +                                goto no_memory;
>> +                        }
>> +                        def->mem.dump_core = virDomainMemDumpTypeFromString(tmp);
>> +                        if (def->mem.dump_core < 0)
> 
> I guess qemu refuses to start with dump-guest-core=default but no one
> knows. s/</<=/
> 

Fixed.

>> +                            def->mem.dump_core = VIR_DOMAIN_MEM_DUMP_DEFAULT;
>> +                        if (params)
>> +                            VIR_FREE(tmp);
>> +                    }
>> +                }
>> +            }
>>          } else if (STREQ(arg, "-serial")) {
>>              WANT_VALUE();
>>              if (STRNEQ(val, "none")) {
>> diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c
>> index a3d9656..079aef8 100644
>> --- a/tests/qemuhelptest.c
>> +++ b/tests/qemuhelptest.c
>> @@ -846,7 +846,8 @@ mymain(void)
>>              QEMU_CAPS_VIRTIO_SCSI_PCI,
>>              QEMU_CAPS_BLOCKIO,
>>              QEMU_CAPS_SCSI_DISK_WWN,
>> -            QEMU_CAPS_SECCOMP_SANDBOX);
>> +            QEMU_CAPS_SECCOMP_SANDBOX,
>> +            QEMU_CAPS_DUMP_GUEST_CORE);
>>
>>      return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
>>  }
>>
> 
> ACK with nits fixed.
> 
> Michal
> 

All fixed, checks are OK. Pushed now, thanks.

Martin


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