[libvirt] [RFC PATCH v2 4/4] PowerPC : Bifurcate arch-specific qemu initialization code from generic qemu command line generation.
Daniel P. Berrange
berrange at redhat.com
Tue Nov 15 11:03:58 UTC 2011
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 at 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 at 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",
> "handle");
>
> +virQemuCommandLineFunction qemuCmdLineX86 =
> + { .qemuBuildArchFunction = qemuBuildX86CommandLine,
> + };
> +
> +virQemuCommandLineFunction qemuCmdLinePPC64 =
> + { .qemuBuildArchFunction = NULL,
> + };
>
> static void
> uname_normalize (struct utsname *ut)
> @@ -3254,6 +3261,8 @@ qemuBuildCommandLine(virConnectPtr conn,
> bool emitBootindex = false;
> int usbcontroller = 0;
> bool usblegacy = false;
> + char *command_str;
> + virQemuCommandLineFunctionPtr qemuCmdFunc;
> uname_normalize(&ut);
>
> virUUIDFormat(def->uuid, uuid);
> @@ -3409,54 +3418,22 @@ qemuBuildCommandLine(virConnectPtr conn,
> }
> }
>
> - if ((def->os.smbios_mode != VIR_DOMAIN_SMBIOS_NONE) &&
> - (def->os.smbios_mode != VIR_DOMAIN_SMBIOS_EMULATE)) {
> - virSysinfoDefPtr source = NULL;
> - bool skip_uuid = false;
> -
> - if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SMBIOS_TYPE)) {
> - qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("the QEMU binary %s does not support smbios settings"),
> - emulator);
> - goto error;
> - }
> -
> - /* should we really error out or just warn in those cases ? */
> - if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_HOST) {
> - if (driver->hostsysinfo == NULL) {
> - qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("Host SMBIOS information is not available"));
> - goto error;
> - }
> - source = driver->hostsysinfo;
> - /* Host and guest uuid must differ, by definition of UUID. */
> - skip_uuid = true;
> - } else if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_SYSINFO) {
> - if (def->sysinfo == NULL) {
> - qemuReportError(VIR_ERR_XML_ERROR,
> - _("Domain '%s' sysinfo are not available"),
> - def->name);
> - goto error;
> - }
> - source = def->sysinfo;
> - /* domain_conf guaranteed that system_uuid matches guest uuid. */
> - }
> - 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
> + 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.
> + command_str = (qemuCmdFunc->qemuBuildArchFunction)(conn, driver, def,
> + qemuCaps);
> + if (command_str) {
> + virCommandAddArg(cmd, command_str);
> + VIR_FREE(command_str);
> + }
> }
> -
> /*
> * NB, -nographic *MUST* come before any serial, or monitor
> * or parallel port flags due to QEMU craziness, where it
> @@ -3475,25 +3452,6 @@ qemuBuildCommandLine(virConnectPtr conn,
> "-nodefaults"); /* Disable default guest devices */
> }
>
> - /* Serial graphics adapter */
> - if (def->os.bios.useserial == VIR_DOMAIN_BIOS_USESERIAL_YES) {
> - if (!qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("qemu does not support -device"));
> - goto error;
> - }
> - if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SGA)) {
> - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("qemu does not support SGA"));
> - goto error;
> - }
> - if (!def->nserials) {
> - qemuReportError(VIR_ERR_XML_ERROR, "%s",
> - _("need at least one serial port to use SGA"));
> - goto error;
> - }
> - virCommandAddArgList(cmd, "-device", "sga", NULL);
> - }
>
> if (monitor_chr) {
> char *chrdev;
> @@ -3664,9 +3622,6 @@ qemuBuildCommandLine(virConnectPtr conn,
> if (monitor_json && qemuCapsGet(qemuCaps, QEMU_CAPS_NO_SHUTDOWN))
> virCommandAddArg(cmd, "-no-shutdown");
>
> - if (!(def->features & (1 << VIR_DOMAIN_FEATURE_ACPI)))
> - virCommandAddArg(cmd, "-no-acpi");
> -
> if (!def->os.bootloader) {
> /*
> * We prefer using explicit bootindex=N parameters for predictable
> @@ -4347,8 +4302,10 @@ qemuBuildCommandLine(virConnectPtr conn,
> VIR_FREE(devstr);
>
> virCommandAddArg(cmd, "-device");
> - virCommandAddArgFormat(cmd, "isa-serial,chardev=char%s,id=%s",
> - serial->info.alias, serial->info.alias);
> + if (!(devstr = qemuBuildChrDeviceStr(serial, def->os.arch)))
> + goto error;
> + virCommandAddArg(cmd, devstr);
> + VIR_FREE(devstr);
> } else {
> virCommandAddArg(cmd, "-serial");
> if (!(devstr = qemuBuildChrArgStr(&serial->source, NULL)))
> @@ -5238,7 +5195,127 @@ qemuBuildCommandLine(virConnectPtr conn,
> return NULL;
> }
>
> +/* This function separates x86* specific initializations in QEMU command
> + * line.
> + */
> +char *
> +qemuBuildX86CommandLine ( virConnectPtr conn,
> + struct qemud_driver *driver,
> + virDomainDefPtr def,
> + virBitmapPtr qemuCaps)
> +{
> + virBuffer opt = VIR_BUFFER_INITIALIZER;
> +
> + if ((def->os.smbios_mode != VIR_DOMAIN_SMBIOS_NONE) &&
> + (def->os.smbios_mode != VIR_DOMAIN_SMBIOS_EMULATE)) {
> + virSysinfoDefPtr source = NULL;
> + bool skip_uuid = false;
>
> + if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SMBIOS_TYPE)) {
> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("the QEMU binary %s does not support smbios settings"),
> + def->emulator);
> + goto error;
> + }
> +
> + /* should we really error out or just warn in those cases ? */
> + if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_HOST) {
> + if (driver->hostsysinfo == NULL) {
> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Host SMBIOS information is not available"));
> + goto error;
> + }
> + source = driver->hostsysinfo;
> + /* Host and guest uuid must differ, by definition of UUID. */
> + skip_uuid = true;
> + } else if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_SYSINFO) {
> + if (def->sysinfo == NULL) {
> + qemuReportError(VIR_ERR_XML_ERROR,
> + _("Domain '%s' sysinfo are not available"),
> + def->name);
> + goto error;
> + }
> + source = def->sysinfo;
> + /* domain_conf guaranteed that system_uuid matches guest uuid. */
> + }
> + if (source != NULL) {
> + char *smbioscmd;
> +
> + smbioscmd = qemuBuildSmbiosBiosStr(source);
> + if (smbioscmd != NULL) {
> + virBufferAsprintf(&opt, ", -smbios %s", smbioscmd);
> + VIR_FREE(smbioscmd);
> + }
> + smbioscmd = qemuBuildSmbiosSystemStr(source, skip_uuid);
> + if (smbioscmd != NULL) {
> + virBufferAsprintf(&opt, ", -smbios", smbioscmd, NULL);
> + VIR_FREE(smbioscmd);
> + }
> + }
> + }
> +
> + /* Serial graphics adapter */
> + if (def->os.bios.useserial == VIR_DOMAIN_BIOS_USESERIAL_YES) {
> + if (!qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("qemu does not support -device"));
> + goto error;
> + }
> + if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SGA)) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("qemu does not support SGA"));
> + goto error;
> + }
> + if (!def->nserials) {
> + qemuReportError(VIR_ERR_XML_ERROR, "%s",
> + _("need at least one serial port to use SGA"));
> + goto error;
> + }
> + virBufferAddLit(&opt, "-device sga");
> + }
> +
> + if (!(def->features & (1 << VIR_DOMAIN_FEATURE_ACPI)))
> + virBufferAddLit(&opt, "-no-acpi");
> +
> + if (virBufferError(&opt)) {
> + virReportOOMError();
> + goto error;
> + }
> +
> + return virBufferContentAndReset(&opt);
> +
> + error:
> + virBufferFreeAndReset(&opt);
> + return NULL;
> +}
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.
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 ?
> +/*
> + * This function generates the correct '-device' string
> + * for each arch.
> + */
> +char*
> +qemuBuildChrDeviceStr (virDomainChrDefPtr serial, char *os_arch)
> +{
> + virBuffer cmd = VIR_BUFFER_INITIALIZER;
> +
> + if (STREQ(os_arch, "ppc64"))
> + virBufferAsprintf(&cmd, " spapr-vty,chardev=char%s ",
> + serial->info.alias);
> + else
> + virBufferAsprintf(&cmd, "isa-serial,chardev=char%s,id=%s",
> + serial->info.alias, serial->info.alias);
> +
> + if (virBufferError(&cmd)) {
> + virReportOOMError();
> + goto error;
> + }
> +
> + return virBufferContentAndReset(&cmd);
> +
> + error:
> + virBufferFreeAndReset(&cmd);
> + return NULL;
> +}
> /*
> * This method takes a string representing a QEMU command line ARGV set
> * optionally prefixed by a list of environment variables. It then tries
> @@ -6444,7 +6521,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> def->maxvcpus = 1;
> def->vcpus = 1;
> def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC;
> - def->features = (1 << VIR_DOMAIN_FEATURE_ACPI)
> + if (!strcmp(def->os.arch, "i686") || !strcmp(def->os.arch, "x86_64"))
> + def->features = (1 << VIR_DOMAIN_FEATURE_ACPI)
> /*| (1 << VIR_DOMAIN_FEATURE_APIC)*/;
> def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART;
> def->onCrash = VIR_DOMAIN_LIFECYCLE_DESTROY;
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index bfdaff9..9694962 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -40,6 +40,16 @@
> # define QEMU_VNC_PORT_MIN 5900
> # define QEMU_VNC_PORT_MAX 65535
>
> +struct _virQemuCommandLineFunction {
> + char * (*qemuBuildArchFunction) (virConnectPtr,
> + struct qemud_driver *,
> + virDomainDefPtr,
> + virBitmapPtr);
> + /* Arch-specific features, such as SMBIOS, ACPI */
> +};
> +
> +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'.
> @@ -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.
> +
> +/* Generate string for arch-specific '-device' parameter */
> +char*
> +qemuBuildChrDeviceStr (virDomainChrDefPtr serial, char *os_arch);
> +
> /* With vlan == -1, use netdev syntax, else old hostnet */
> char * qemuBuildHostNetStr(virDomainNetDefPtr net,
> char type_sep,
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 :|
More information about the libvir-list
mailing list