[libvirt] [PATCHv2 1/2] Optimize machine option to set more options with it.

[Martin Kletzander]

On 03/15/2013 10:19 AM, Li Zhang wrote:
> From: Li Zhang <zhlcindy at linux.vnet.ibm.com>
> 
>  Currently, -machine option is used only when dump-guest-core is set.
> 
>  To use options defined in machine option for newer version of QEMU,
>  it needs to use -machine xxx, and to be compatible with older version
>  -M, this patch addes QEMU_CAPS_MACHINE_OPT capability for newer version,
>  say 1.2.0.
> 
> Signed-off-by: Li Zhang <zhlcindy at linux.vnet.ibm.com>
> ---
>  v2 -> v1:
>   * Split the patch to 2 parts suggested by Daniel P.Berrange
>   * Rename QEMU_CAPS_MACH_OPT to QEMU_CAPS_MACHINE_OPT
>   * Remove version 1.1 assertion for QEMU_CAPS_MACHINE_OPT   
> 
>  src/qemu/qemu_capabilities.c |    6 +++++-
>  src/qemu/qemu_capabilities.h |    1 +
>  src/qemu/qemu_command.c      |   30 +++++++++++++++++++-----------
>  tests/qemuxml2argvtest.c     |    6 +++---
>  4 files changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 519d2c5..778e825 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -210,7 +210,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>  
>                "rng-random", /* 130 */
>                "rng-egd",
> -              "virtio-ccw"
> +              "virtio-ccw",
> +              "machine-opt"
>      );
>  
>  struct _virQEMUCaps {
> @@ -2442,6 +2443,9 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>  
>      virQEMUCapsInitQMPBasic(qemuCaps);
>  
> +    /* machine option is supported for newer version */
> +    virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_OPT);
> +

But this is also supported when we don't probe by QMP.

>      if (!(archstr = qemuMonitorGetTargetArch(mon)))
>          goto cleanup;
>  
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index da06e27..66df556 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -172,6 +172,7 @@ enum virQEMUCapsFlags {
>                                             virtio rng */
>      QEMU_CAPS_OBJECT_RNG_EGD     = 131, /* EGD protocol daemon for rng */
>      QEMU_CAPS_VIRTIO_CCW         = 132, /* -device virtio-*-ccw */
> +    QEMU_CAPS_MACHINE_OPT        = 133, /* -machine xxxx*/
>  
>      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 dc49d44..c39faf0 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4941,6 +4941,8 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
>                         const virDomainDefPtr def,
>                         virQEMUCapsPtr qemuCaps)
>  {
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
>      /* 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
> @@ -4948,27 +4950,33 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
>      if (!def->os.machine)
>          return 0;
>  
> -    if (!def->mem.dump_core) {
> +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_OPT)) {

This was intentionally done the way you see.  I tried to avoid using
'-machine' for various other qemu implementations (I couldn't find
anywhere that '-machine' is guaranteed on all sorts of implementations).
 This is why the following comment is in place, which you obsoleted, but
haven't removed, so it doesn't make much sense now.

Also this won't work with implementations where we don't probe by QMP,
but '-machine dump-core=xx' is supported.

>          /* 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 {
> -        if (!virQEMUCapsGet(qemuCaps, 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,dump-guest-core=%s",
> -                               def->os.machine,
> -                               virDomainMemDumpTypeToString(def->mem.dump_core));
> +        virBufferAsprintf(&buf, "%s", def->os.machine);
> +
> +        if (def->mem.dump_core) {
> +            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE)) {

There is also on pre-existing bug in my code which I see now.  When
probing by QMP, we don't set QEMU_CAPS_DUMP_GUEST_CORE flag and hence
report this as unsupported even when the qemu knows this option :(

However, I believe we have to do some more investigation and rely on
version-specific capabilities only as a last resort.  But because there
is no way to get information about ',usb=off' and more of these are
already in the code and coming, we will probably need to switch to
'-machine' anyway.

Replying to your [2/2] here as well, with what kind of configuration do
you experience such problems?  I see no problems with upstream git QEMU
(reporting itself as version 1.4.50), nor 1.4.0.

Thanks,
Martin