Re: [PATCHv2 1/1][RESEND] Optimize machine option to set more options with it.

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

 



On Thu, Mar 14, 2013 at 02:54:20PM +0800, Li Zhang wrote:
> From: Li Zhang <zhlcindy@xxxxxxxxxxxxxxxxxx>
> 
> Currently, -machine option is used only when dump-guest-core is used.
> 
> To use options defined in machine option for new version of QEMU,
> it needs to use -machine xxx, and to be compatible with older version
>  -M, this patch addes QEMU_CAPS_MACH_OPT capability, and assumes
>  -machine is used for QEMU v1.0 onwards.
> 
> To avoid the collision for creating USB controllers when using USB
> option and -device xxxx, it needs to set usb=off in machine option.
> QEMU_CAPS_USB_OPT capability is added, and it will be for QEMU
> v1.3.0-rc0 onwards which supports USB option.

I'd rather see this patch split into two parts. One part to introduce
the use of -machine, and a second patch to deal with the usb=off part.

That said I'm not sure why we need to set usb=off at all - we already run
qemu with -nodefconfig -nodefaults which should be blocking the default
USB controller.

> Signed-off-by: Li Zhang <zhlcindy@xxxxxxxxxxxxxxxxxx>
> ---
>  src/qemu/qemu_capabilities.c |   10 ++++++++++
>  src/qemu/qemu_capabilities.h |    2 ++
>  src/qemu/qemu_command.c      |   36 +++++++++++++++++++++++++-----------
>  tests/qemuxml2argvtest.c     |    4 ++--
>  4 files changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 51fc9dc..79eb83f 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -205,6 +205,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>                "usb-serial", /* 125 */
>                "usb-net",
>                "add-fd",
> +              "mach-opt",
> +              "usb-opt",
>  
>      );
>  
> @@ -2416,6 +2418,14 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>  
>      virQEMUCapsInitQMPBasic(qemuCaps);
>  
> +    /* Assuming to use machine option v1.0 onwards*/
> +    if (qemuCaps->version >= 1000000)
> +        virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACH_OPT);
> +
> +    /* USB option is supported v1.3.0-rc0 onwards */
> +    if (qemuCaps->version >= 1002090)
> +        virQEMUCapsSet(qemuCaps, QEMU_CAPS_USB_OPT);
> +
>      if (!(archstr = qemuMonitorGetTargetArch(mon)))
>          goto cleanup;

If we get to this aprt of virQEMUCapsInitQMP() code path, then we already
know we have a QEMU >= 1.2.0, so this first version check is not required.
Just set the QEMU_CAPS_MACH_OPT in virQEMUCapsInitQMPBasic.

For the USB property just use version 1.3.0 - don't try to do stuff
based on development snapshot version numbers.

> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index e69d558..06aaa68 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -166,6 +166,8 @@ enum virQEMUCapsFlags {
>      QEMU_CAPS_DEVICE_USB_SERIAL  = 125, /* -device usb-serial */
>      QEMU_CAPS_DEVICE_USB_NET     = 126, /* -device usb-net */
>      QEMU_CAPS_ADD_FD             = 127, /* -add-fd */
> +    QEMU_CAPS_MACH_OPT           = 128, /* -machine xxxx*/

We don't need to abbreviate - use QEMU_CAPS_MACHINE_OPT

> +    QEMU_CAPS_USB_OPT            = 129, /* -machine xxxx,usb=off*/

I'd prefer this second one to be  QEMU_CAPS_MACHINE_USB_OPT

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6c28123..e7dde21 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4614,6 +4614,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
> @@ -4621,27 +4623,39 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
>      if (!def->os.machine)
>          return 0;
>  
> -    if (!def->mem.dump_core) {
> +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACH_OPT)) {
>          /* 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);
> +
> +        /* To avoid the collision of creating USB controllers when calling
> +         * machine->init in QEMU, it needs to set usb=off
> +         */
> +        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_USB_OPT))
> +            virBufferAsprintf(&buf, ",usb=off");
> +
> +        if (def->mem.dump_core) {
> +            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;
> +            }
> +
> +            virBufferAsprintf(&buf, ",dump-guest-core=%s",
> +                              virDomainMemDumpTypeToString(def->mem.dump_core));
> +        }
> +
> +        virCommandAddArg(cmd, virBufferContentAndReset(&buf));
>      }
>  

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

--
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]