12:16 p.m.
Hi Daniel,
Thanks for taking a look.
On 11/15/2011 04:33 PM, Daniel P. Berrange wrote:
On Mon, Nov 14, 2011 at 08:26:55PM +0530, Prerna Saxena wrote:
> From ae7764be4454be1900fc3ee0a03bf819d5cd12de Mon Sep 17 00:00:00 2001
> From: Prerna Saxena<prerna(a)linux.vnet.ibm.com>
> Date: Mon, 14 Nov 2011 19:43:26 +0530
> Subject: [PATCH 4/4] Separate out arch-specific qemu initialization from
> generic qemu commandline building code.
> At present, qemuBuildCommandLine emits many x86-specific options while
> generating the qemu command line. This patch proposes a framework to add
> arch-specific handler functions for generating different aspects of
> command line.
> At present, it is used to prevent x86-specific qemu options from
> cluttering the qemu command line for other architectures. Going forward,
> if newer drivers get defined for any arch, the code for handling these
> might be added by extending the handler function for that architecture.
>
> Signed-off-by: Prerna Saxena<prerna(a)linux.vnet.ibm.com>
> ---
> src/qemu/qemu_command.c | 220 ++++++++++++++++++++++++++++++++---------------
> src/qemu/qemu_command.h | 20 ++++
> 2 files changed, 169 insertions(+), 71 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b044050..61dabbf 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -107,6 +107,13 @@ VIR_ENUM_IMPL(qemuDomainFSDriver,
VIR_DOMAIN_FS_DRIVER_TYPE_LAST,
> "local",
> ...
> [snip]
> ...
> - if (source != NULL) {
> - char *smbioscmd;
> + if (!strcmp(def->os.arch, "i686") ||
> + !strcmp(def->os.arch, "x86_64"))
The coding guidelines require use of STREQ or STRNEQ
I'm sorry, overlooked this. I'll correct this to use STREQ.
> + qemuCmdFunc =&qemuCmdLineX86;
> + else {
> + if (!strcmp(def->os.arch, "ppc64"))
> + qemuCmdFunc =&qemuCmdLinePPC64;
> + }
If we get an arch that is not PPC or x86, then 'qemuCmdFunc'
can end up NULL
>
> - smbioscmd = qemuBuildSmbiosBiosStr(source);
> - if (smbioscmd != NULL) {
> - virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL);
> - VIR_FREE(smbioscmd);
> - }
> - smbioscmd = qemuBuildSmbiosSystemStr(source, skip_uuid);
> - if (smbioscmd != NULL) {
> - virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL);
> - VIR_FREE(smbioscmd);
> - }
> - }
> + if (qemuCmdFunc->qemuBuildArchFunction) {
Which will cause a SEGV here.
My bad, I should have had something like:
if (qemuCmdFunc && qemuCmdFunc->qemubuildArchFunction)
...
I'll correct this.
> ...
> [snip]
> ....
>
The convention we try to follow is that if there is something in the
XML which is not supported by the QEMU we're about to run, then we
should raise an error VIR_ERR_CONFIG_UNSUPPORTED.
Thus I'd expect to see a PPC method which raises such an error if
the user requests no-ACPI, serial BIOS output, or SMBIOS data.
Thank you for explaining this.
I get your point -- libvirt should be able to flag an error if the user
tries to specify in XML, a feature unsupported by qemu.
To check if qemu supports a certain feature, libvirt parses the -help
string which is generated by running the qemu binary with the '-h'
argument.
Example: For libvirt to determine if '-no-acpi' is a valid option, it
parses the qemu help string to determine if '-no-acpi' is a supported
feature. If this option is found in the help string, libvirt assumes
that this is an allowed option.
However, qemu itself has an inherent limitation. When generating the
list of allowed options with the '-h' flags, qemu blindly lists all
options instead of doing any arch-specific checking. Example, 'no-acpi'
is defined with an architecture mask for only x86.
$cat $QEMU_GIT/qemu-options.hx :
.....
DEF("no-acpi", 0, QEMU_OPTION_no_acpi,
"-no-acpi disable ACPI\n", QEMU_ARCH_I386)
STEXI
@item -no-acpi
@findex -no-acpi
Disable ACPI (Advanced Configuration and Power Interface) support. Use
it if your guest OS complains about ACPI problems (PC target machine
only).
ETEXI
....
However, the arch-specific masks are not checked while generating the
help string.
$cat $QEMU_GIT/vl.c:
...
const char *options_help =
#define DEF(option, opt_arg, opt_enum, opt_help, arch_mask) \
opt_help
#define DEFHEADING(text) stringify(text) "\n"
#include "qemu-options.def"
....
(where qemu-options.def is generated from qemu-options.hx)
This essentially means that the 'help' string emitted by qemu is
identical, irrespective of the architecture.
So, even if a qemu binary meant for a non-x86 architecture may support a
subset of qemu's x86 features, the qemu binary for that architecture
will always display an identical help string as x86-- never advertising
its true features. This will trick libvirt, because launching
qemu-system-ppc64 or qemu-system-mips with '-no-acpi' errors out with:
"Option no-acpi not supported for this target"
There is also a second part of this problem. Libvirt till now has been
x86-based only, and so it probes host-architecture qemu to gather
supported features, and continues to use these while validating a
guest's features.
For example, libvirt should make a decision about processing 'acpi' XML
feature only if the guest architecture fundamentally supports this.
At present, libvirt on x86 assumes acpi to be a mandatory feature in any
guest XML. In the absence of the <acpi> feature, it adds 'no-acpi' in
the qemu command line.
On an x86 host, while this might be fine for a guest running via
qemu-system-x86_64 , it will not be right for a guest running via
qemu-system-sparc.
Considering libvirt is potentially capable of launching pure emulation
based cross-architecture guests, it would be an incorrect assumption.
The decision to even process the 'acpi' tag should be taken based on
whether the guest architecture supports this -- this patch attempts to
accomplish this.
This patch transfers the decision of processing an XML tag to an
arch-specific handler. In the absence of this handler, the option is
silently ignored.
Finally, looking at the changes, I'd be somewhat amazed if this
refactoring has not broken the test suite since AFAICT, it changes
the ordering of some command line arguments. Did 'make check'
really pass after applying this ?
I'm sorry, I'd tested this by hand and did not run a 'make check' at
the time. Ran it now to see that 59 tests pass and 3 tests fail.
I will remember to run 'make check' henceforth.
> ...
> [snip]
> ...
> +typedef struct _virQemuCommandLineFunction virQemuCommandLineFunction;
> +typedef struct _virQemuCommandLineFunction* virQemuCommandLineFunctionPtr;
These can both be in the source file too. Also the naming convention for
the QEMU driver source files is a prefix 'qemu' ather than 'virQemu'.
Agree, I'll address this.
> @@ -53,6 +63,16 @@ virCommandPtr
qemuBuildCommandLine(virConnectPtr conn,
> enum virVMOperationType vmop)
> ATTRIBUTE_NONNULL(1);
>
> +/* Build Additional X86-specific options on command line */
> +char * qemuBuildX86CommandLine(virConnectPtr conn,
> + struct qemud_driver *driver,
> + virDomainDefPtr def,
> + virBitmapPtr qemuCaps);
I think this function can just be static - there's no need to be able
to access it outside the source file it lives in.
Will do.
..[snip]..
Shared my understanding on the present limitations of qemu / libvirt;
and the need for an arch-based cleanup patch. I would love to stand
corrected if I have overlooked any aspect. It will also be nice to hear
of suggestions on how I can organize this design better.
Thanks,
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India