[libvirt] [PATCH v3 1/2] Optimize machine option to set more options with it
From: Li Zhang <zhlcindy@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 which supports -machine option. Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- v3 -> v2: * Set QEMU_CAPS_MACHINE_OPT with help string * Set QEMU_CAPS_DUMP_GUEST_CORE with QMP 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 | 11 +++++++++++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 32 ++++++++++++++++++-------------- tests/qemuhelptest.c | 4 ++++ tests/qemuxml2argvtest.c | 6 +++--- 5 files changed, 37 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 861d3c4..7d459db 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -213,6 +213,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-ccw", "dtb", "megasas", + + "machine-opt", /* 135 */ ); struct _virQEMUCaps { @@ -1091,6 +1093,9 @@ virQEMUCapsComputeCmdFlags(const char *help, if (strstr(help, "-dtb")) virQEMUCapsSet(qemuCaps, QEMU_CAPS_DTB); + if (strstr(help, "-machine")) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_OPT); + /* * Handling of -incoming arg with varying features * -incoming tcp (kvm >= 79, qemu >= 0.10.0) @@ -2442,6 +2447,12 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, virQEMUCapsInitQMPBasic(qemuCaps); + /* machine option is supported for newer version */ + virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_OPT); + + /* -dump-guest-core is supported for newer version*/ + virQEMUCapsSet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE); + if (!(archstr = qemuMonitorGetTargetArch(mon))) goto cleanup; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 7101f67..2595868 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -174,6 +174,7 @@ enum virQEMUCapsFlags { QEMU_CAPS_VIRTIO_CCW = 132, /* -device virtio-*-ccw */ QEMU_CAPS_DTB = 133, /* -dtb file */ QEMU_CAPS_SCSI_MEGASAS = 134, /* -device megasas */ + QEMU_CAPS_MACHINE_OPT = 135, /* -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 a0c278f..0ee634b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5197,6 +5197,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 @@ -5204,27 +5206,29 @@ qemuBuildMachineArgStr(virCommandPtr cmd, if (!def->os.machine) return 0; - if (!def->mem.dump_core) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_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); + + 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)); } return 0; diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index a28109a..0c7e607 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -718,6 +718,7 @@ mymain(void) QEMU_CAPS_SCSI_LSI, QEMU_CAPS_BLOCKIO, QEMU_CAPS_VNC, + QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_DEVICE_QXL, QEMU_CAPS_DEVICE_VGA, QEMU_CAPS_DEVICE_CIRRUS_VGA, @@ -806,6 +807,7 @@ mymain(void) QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_BLOCKIO, QEMU_CAPS_VNC, + QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_DEVICE_QXL, QEMU_CAPS_DEVICE_VGA, QEMU_CAPS_DEVICE_CIRRUS_VGA, @@ -903,6 +905,7 @@ mymain(void) QEMU_CAPS_SECCOMP_SANDBOX, QEMU_CAPS_DUMP_GUEST_CORE, QEMU_CAPS_VNC, + QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_USB_REDIR_BOOTINDEX, QEMU_CAPS_USB_HOST_BOOTINDEX, QEMU_CAPS_DEVICE_QXL, @@ -1009,6 +1012,7 @@ mymain(void) QEMU_CAPS_SECCOMP_SANDBOX, QEMU_CAPS_DUMP_GUEST_CORE, QEMU_CAPS_VNC, + QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_USB_REDIR_BOOTINDEX, QEMU_CAPS_USB_HOST_BOOTINDEX, QEMU_CAPS_DEVICE_QXL, diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 38787ac..4b45c55 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -363,9 +363,9 @@ mymain(void) DO_TEST("minimal-s390", QEMU_CAPS_NAME); DO_TEST("machine-aliases1", NONE); DO_TEST("machine-aliases2", QEMU_CAPS_KVM); - DO_TEST("machine-core-on", QEMU_CAPS_DUMP_GUEST_CORE); - DO_TEST("machine-core-off", QEMU_CAPS_DUMP_GUEST_CORE); - DO_TEST_FAILURE("machine-core-on", NONE); + DO_TEST("machine-core-on", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_DUMP_GUEST_CORE); + DO_TEST("machine-core-off", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_DUMP_GUEST_CORE); + DO_TEST_FAILURE("machine-core-on", QEMU_CAPS_MACHINE_OPT, NONE); DO_TEST("boot-cdrom", NONE); DO_TEST("boot-network", NONE); DO_TEST("boot-floppy", NONE); -- 1.7.10.1
From: Li Zhang <zhlcindy@linux.vnet.ibm.com> To avoid the collision for creating USB controllers in machine->init() and -device xx command line, it needs to set usb=off to avoid one USB controller created in machine->init(). So that libvirt can use -device or -usb to create USB controller sucessfully. So QEMU_CAPS_MACHINE_USB_OPT capability is added, and it is for QEMU v1.3.0 onwards which supports USB option. Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- v3 -> v2: * Set QEMU_CAPS_MACHINE_USB_OPT with help string v2 -> v1: * Rename QEMU_CAPS_USB_OPT to QEMU_CAPS_MACHINE_USB_OPT suggested by Daniel * Corret QEMU version with v1.3.0 suggested by Daniel src/qemu/qemu_capabilities.c | 9 +++++++++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 6 ++++++ 3 files changed, 16 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 7d459db..a5f8944 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -215,6 +215,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "megasas", "machine-opt", /* 135 */ + "machine-usb-opt", ); struct _virQEMUCaps { @@ -1096,6 +1097,10 @@ virQEMUCapsComputeCmdFlags(const char *help, if (strstr(help, "-machine")) virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_OPT); + /* USB option is supported v1.3.0 onwards */ + if (qemuCaps->version >= 1003000) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_USB_OPT); + /* * Handling of -incoming arg with varying features * -incoming tcp (kvm >= 79, qemu >= 0.10.0) @@ -2453,6 +2458,10 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, /* -dump-guest-core is supported for newer version*/ virQEMUCapsSet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE); + /* USB option is supported v1.3.0 onwards */ + if (qemuCaps->version >= 1003000) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_USB_OPT); + if (!(archstr = qemuMonitorGetTargetArch(mon))) goto cleanup; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 2595868..091fe6e7 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -175,6 +175,7 @@ enum virQEMUCapsFlags { QEMU_CAPS_DTB = 133, /* -dtb file */ QEMU_CAPS_SCSI_MEGASAS = 134, /* -device megasas */ QEMU_CAPS_MACHINE_OPT = 135, /* -machine xxxx*/ + QEMU_CAPS_MACHINE_USB_OPT = 136, /* -machine xxx,usb=on/off */ 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 0ee634b..2d9398b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5216,6 +5216,12 @@ qemuBuildMachineArgStr(virCommandPtr cmd, virCommandAddArg(cmd, "-machine"); 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_MACHINE_USB_OPT)) + virBufferAsprintf(&buf, ",usb=off"); + if (def->mem.dump_core) { if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, -- 1.7.10.1
On Fri, Mar 29, 2013 at 01:22:47PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
To avoid the collision for creating USB controllers in machine->init() and -device xx command line, it needs to set usb=off to avoid one USB controller created in machine->init(). So that libvirt can use -device or -usb to create USB controller sucessfully. So QEMU_CAPS_MACHINE_USB_OPT capability is added, and it is for QEMU v1.3.0 onwards which supports USB option.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
I'm not seeing why this is needed - we pass -nodefconfig and -nodefaults which ought to disable any default built-in USB controller already surely ? 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 :|
On 2013年04月02日 18:06, Daniel P. Berrange wrote:
On Fri, Mar 29, 2013 at 01:22:47PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
To avoid the collision for creating USB controllers in machine->init() and -device xx command line, it needs to set usb=off to avoid one USB controller created in machine->init(). So that libvirt can use -device or -usb to create USB controller sucessfully. So QEMU_CAPS_MACHINE_USB_OPT capability is added, and it is for QEMU v1.3.0 onwards which supports USB option.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
I'm not seeing why this is needed - we pass -nodefconfig and -nodefaults which ought to disable any default built-in USB controller already surely ?
With -nodefconfig and -nodefaults, QEMU doesn't disable USB controller. In QEMU, USB controller will be created according to usb_enabled(default). This the default value can be set according to different platforms. For example, in pSeries guest, Because there are no ps2 devices, so it needs USB keyboard and mouse with VGA. So, USB controller will be created with graphic enabled in machine->init(). if (usb_enabled(spapr->has_graphics)) { pci_create_simple(phb->bus, -1, "pci-ohci"); if (spapr->has_graphics) { usbdevice_create("keyboard"); usbdevice_create("mouse"); } } Libvirt can't know this controller which is created by QEMU. So it will report error. :)
Daniel
On Wed, Apr 03, 2013 at 10:14:15AM +0800, Li Zhang wrote:
On 2013年04月02日 18:06, Daniel P. Berrange wrote:
On Fri, Mar 29, 2013 at 01:22:47PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
To avoid the collision for creating USB controllers in machine->init() and -device xx command line, it needs to set usb=off to avoid one USB controller created in machine->init(). So that libvirt can use -device or -usb to create USB controller sucessfully. So QEMU_CAPS_MACHINE_USB_OPT capability is added, and it is for QEMU v1.3.0 onwards which supports USB option.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
I'm not seeing why this is needed - we pass -nodefconfig and -nodefaults which ought to disable any default built-in USB controller already surely ?
With -nodefconfig and -nodefaults, QEMU doesn't disable USB controller.
In QEMU, USB controller will be created according to usb_enabled(default). This the default value can be set according to different platforms. For example, in pSeries guest, Because there are no ps2 devices, so it needs USB keyboard and mouse with VGA. So, USB controller will be created with graphic enabled in machine->init().
if (usb_enabled(spapr->has_graphics)) { pci_create_simple(phb->bus, -1, "pci-ohci"); if (spapr->has_graphics) { usbdevice_create("keyboard"); usbdevice_create("mouse"); } }
Libvirt can't know this controller which is created by QEMU. So it will report error. :)
Ok, I see what's going on now. This is horrible QEMU behaviour, but we have to live with it. So I'm ok with your proposed patch. Before we can commit it though, either you need to update some existing test case, or add a new test case that covers this CLI option. Regards, 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 :|
On 2013年04月04日 00:20, Daniel P. Berrange wrote:
On Wed, Apr 03, 2013 at 10:14:15AM +0800, Li Zhang wrote:
On 2013年04月02日 18:06, Daniel P. Berrange wrote:
On Fri, Mar 29, 2013 at 01:22:47PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
To avoid the collision for creating USB controllers in machine->init() and -device xx command line, it needs to set usb=off to avoid one USB controller created in machine->init(). So that libvirt can use -device or -usb to create USB controller sucessfully. So QEMU_CAPS_MACHINE_USB_OPT capability is added, and it is for QEMU v1.3.0 onwards which supports USB option.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> I'm not seeing why this is needed - we pass -nodefconfig and -nodefaults which ought to disable any default built-in USB controller already surely ? With -nodefconfig and -nodefaults, QEMU doesn't disable USB controller.
In QEMU, USB controller will be created according to usb_enabled(default). This the default value can be set according to different platforms. For example, in pSeries guest, Because there are no ps2 devices, so it needs USB keyboard and mouse with VGA. So, USB controller will be created with graphic enabled in machine->init().
if (usb_enabled(spapr->has_graphics)) { pci_create_simple(phb->bus, -1, "pci-ohci"); if (spapr->has_graphics) { usbdevice_create("keyboard"); usbdevice_create("mouse"); } }
Libvirt can't know this controller which is created by QEMU. So it will report error. :) Ok, I see what's going on now. This is horrible QEMU behaviour, but we have to live with it.
So I'm ok with your proposed patch. Before we can commit it though, either you need to update some existing test case, or add a new test case that covers this CLI option. OK, I will add one test case for it.
Thanks. :)
Regards, Daniel
On Fri, Mar 29, 2013 at 01:22:46PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@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 which supports -machine option.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- v3 -> v2: * Set QEMU_CAPS_MACHINE_OPT with help string * Set QEMU_CAPS_DUMP_GUEST_CORE with QMP
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 | 11 +++++++++++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 32 ++++++++++++++++++-------------- tests/qemuhelptest.c | 4 ++++ tests/qemuxml2argvtest.c | 6 +++--- 5 files changed, 37 insertions(+), 17 deletions(-)
ACK 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 :|
On 04/02/2013 04:05 AM, Daniel P. Berrange wrote:
On Fri, Mar 29, 2013 at 01:22:46PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@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
s/addes/adds/
version which supports -machine option.
ACK
Pushed, after making some fixes (don't have two spaces in the error message, smaller scope for the buffer, use faster buffer functions, and report an error if dump_guest_core is present with the -M form where it is not supported): diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 14be49f..a6d011e 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -5200,8 +5200,6 @@ 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 @@ -5214,16 +5212,23 @@ qemuBuildMachineArgStr(virCommandPtr cmd, * '-M' to keep the most of the compatibility with older versions. */ virCommandAddArgList(cmd, "-M", def->os.machine, NULL); + if (def->mem.dump_core) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("dump-guest-core is not available " + "with this QEMU binary")); + return -1; + } } else { + virBuffer buf = VIR_BUFFER_INITIALIZER; virCommandAddArg(cmd, "-machine"); - virBufferAsprintf(&buf, "%s", def->os.machine); + virBufferAdd(&buf, def->os.machine, -1); 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")); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("dump-guest-core is not available " + "with this QEMU binary")); return -1; } @@ -5231,7 +5236,7 @@ qemuBuildMachineArgStr(virCommandPtr cmd, virDomainMemDumpTypeToString(def->mem.dump_core)); } - virCommandAddArg(cmd, virBufferContentAndReset(&buf)); + virCommandAddArgBuffer(cmd, &buf); } return 0; -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 2013年04月02日 20:52, Eric Blake wrote:
On 04/02/2013 04:05 AM, Daniel P. Berrange wrote:
On Fri, Mar 29, 2013 at 01:22:46PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@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 s/addes/adds/
version which supports -machine option.
ACK Pushed, after making some fixes (don't have two spaces in the error message, smaller scope for the buffer, use faster buffer functions, and report an error if dump_guest_core is present with the -M form where it is not supported): Thanks a lot. :)
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 14be49f..a6d011e 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -5200,8 +5200,6 @@ 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 @@ -5214,16 +5212,23 @@ qemuBuildMachineArgStr(virCommandPtr cmd, * '-M' to keep the most of the compatibility with older versions. */ virCommandAddArgList(cmd, "-M", def->os.machine, NULL); + if (def->mem.dump_core) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("dump-guest-core is not available " + "with this QEMU binary")); + return -1; + } } else { + virBuffer buf = VIR_BUFFER_INITIALIZER;
virCommandAddArg(cmd, "-machine"); - virBufferAsprintf(&buf, "%s", def->os.machine); + virBufferAdd(&buf, def->os.machine, -1);
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")); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("dump-guest-core is not available " + "with this QEMU binary")); return -1; }
@@ -5231,7 +5236,7 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
virDomainMemDumpTypeToString(def->mem.dump_core)); }
- virCommandAddArg(cmd, virBufferContentAndReset(&buf)); + virCommandAddArgBuffer(cmd, &buf); }
return 0;
On 04/02/2013 09:50 PM, Li Zhang wrote:
On 2013年04月02日 20:52, Eric Blake wrote:
On 04/02/2013 04:05 AM, Daniel P. Berrange wrote:
On Fri, Mar 29, 2013 at 01:22:46PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@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 s/addes/adds/
version which supports -machine option.
ACK Pushed, after making some fixes (don't have two spaces in the error message, smaller scope for the buffer, use faster buffer functions, and report an error if dump_guest_core is present with the -M form where it is not supported): Thanks a lot. :)
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 14be49f..a6d011e 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -5200,8 +5200,6 @@ 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 @@ -5214,16 +5212,23 @@ qemuBuildMachineArgStr(virCommandPtr cmd, * '-M' to keep the most of the compatibility with older versions. */ virCommandAddArgList(cmd, "-M", def->os.machine, NULL); + if (def->mem.dump_core) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("dump-guest-core is not available " + "with this QEMU binary")); + return -1; + } } else { + virBuffer buf = VIR_BUFFER_INITIALIZER;
virCommandAddArg(cmd, "-machine"); - virBufferAsprintf(&buf, "%s", def->os.machine); + virBufferAdd(&buf, def->os.machine, -1);
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")); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("dump-guest-core is not available " + "with this QEMU binary")); return -1; }
@@ -5231,7 +5236,7 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
virDomainMemDumpTypeToString(def->mem.dump_core)); }
- virCommandAddArg(cmd, virBufferContentAndReset(&buf)); + virCommandAddArgBuffer(cmd, &buf); }
Just ran a make -C tests valgrind and got the following which points right at this patch: TEST: qemuxml2argvtest ........................................ 40 ........................................ 80 ........................................ 120 ........................................ 160 ........................................ 200 ........................................ 240 .................................. 274 OK ==6299== 1,004 bytes in 1 blocks are definitely lost in loss record 77 of 83 ==6299== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==6299== by 0x4A089F0: realloc (vg_replace_malloc.c:662) ==6299== by 0x4C6B35E: virReallocN (viralloc.c:184) ==6299== by 0x4C6DD97: virBufferGrow (virbuffer.c:129) ==6299== by 0x4C6DFC9: virBufferAdd (virbuffer.c:165) ==6299== by 0x43097A: qemuBuildCommandLine (qemu_command.c:5224) ==6299== by 0x41E960: testCompareXMLToArgvHelper (qemuxml2argvtest.c:154) ==6299== by 0x41FF4F: virtTestRun (testutils.c:157) ==6299== by 0x416C47: mymain (qemuxml2argvtest.c:371) ==6299== by 0x42058A: virtTestMain (testutils.c:719) ==6299== by 0x38D6821A04: (below main) (in /usr/lib64/libc-2.16.so) ==6299==
return 0;
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/03/2013 10:14 AM, John Ferlan wrote:
} else { + virBuffer buf = VIR_BUFFER_INITIALIZER;
virCommandAddArg(cmd, "-machine"); - virBufferAsprintf(&buf, "%s", def->os.machine); + virBufferAdd(&buf, def->os.machine, -1);
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")); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("dump-guest-core is not available " + "with this QEMU binary")); return -1;
Just ran a make -C tests valgrind and got the following which points right at this patch:
Sheesh - this return -1 is indeed a memory leak. I'll fix it. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan -
Li Zhang