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

On Thu, Mar 14, 2013 at 02:54:20PM +0800, Li Zhang wrote: > From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > > 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(a)linux.vnet.ibm.com&gt; > --- > 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