On 07/31/2013 10:14 PM, Cole Robinson wrote:
> Unlike USB, qemu_command can handle an empty memballoon device, so just
> don't add it.
>
> And add test cases for a basic working ARM guest.
> ---
> docs/schemas/domaincommon.rng | 19 +++++++++++
> src/conf/domain_conf.c | 38 ++++++++++++++--------
> .../qemuxml2argv-arm-vexpressa9-nodevs.args | 1 +
> .../qemuxml2argv-arm-vexpressa9-nodevs.xml | 26 +++++++++++++++
> tests/qemuxml2argvtest.c | 3 ++
> tests/testutilsqemu.c | 32 ++++++++++++++++++
> 6 files changed, 106 insertions(+), 13 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-arm-vexpressa9-nodevs.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-arm-vexpressa9-nodevs.xml
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 745b959..781ecfd 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -303,6 +303,7 @@
> <ref name="hvmppc"/>
> <ref name="hvmppc64"/>
> <ref name="hvms390"/>
> + <ref name="hvmarm"/>
> </choice>
> </optional>
> <value>hvm</value>
> @@ -412,6 +413,24 @@
> </optional>
> </group>
> </define>
> + <define name="hvmarm">
> + <group>
> + <optional>
> + <attribute name="arch">
> + <choice>
> + <value>armv7l</value>
> + </choice>
> + </attribute>
> + </optional>
> + <optional>
> + <attribute name="machine">
> + <data type="string">
> + <param name="pattern">[a-zA-Z0-9_\.\-]+</param>
> + </data>
> + </attribute>
> + </optional>
> + </group>
> + </define>
> <define name="osexe">
> <element name="os">
> <element name="type">
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2308580..9d903f4 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8812,6 +8812,23 @@ virDomainDefaultUSBControllerModel(virDomainDefPtr def)
> return -1;
> }
>
> +static bool
> +virDomainNeedDefaultMemballoonDevice(virDomainDefPtr def)
> +{
> + if (def->virtType == VIR_DOMAIN_VIRT_XEN)
> + return true;
> +
> + if (def->virtType != VIR_DOMAIN_VIRT_QEMU &&
> + def->virtType != VIR_DOMAIN_VIRT_KQEMU &&
> + def->virtType != VIR_DOMAIN_VIRT_KVM)
> + return false;
> +
> + if (def->os.arch == VIR_ARCH_ARMV7L)
> + return false;
> +
> + return true;
> +}
> +
Same comment applies to this as to 2/9 - arch-specific differences
should go in the post-parse callback rather than in the parser itself.
> int
> virDomainVideoDefaultType(virDomainDefPtr def)
> {
> @@ -12156,19 +12173,14 @@ virDomainDefParseXML(xmlDocPtr xml,
>
> def->memballoon = memballoon;
> VIR_FREE(nodes);
> - } else {
> - if (def->virtType == VIR_DOMAIN_VIRT_XEN ||
> - def->virtType == VIR_DOMAIN_VIRT_QEMU ||
> - def->virtType == VIR_DOMAIN_VIRT_KQEMU ||
> - def->virtType == VIR_DOMAIN_VIRT_KVM) {
> - virDomainMemballoonDefPtr memballoon;
> - if (VIR_ALLOC(memballoon) < 0)
> - goto error;
> - memballoon->model = def->virtType == VIR_DOMAIN_VIRT_XEN ?
> - VIR_DOMAIN_MEMBALLOON_MODEL_XEN :
> - VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO;
> - def->memballoon = memballoon;
> - }
> + } else if (virDomainNeedDefaultMemballoonDevice(def)) {
> + virDomainMemballoonDefPtr memballoon;
> + if (VIR_ALLOC(memballoon) < 0)
> + goto error;
> + memballoon->model = def->virtType == VIR_DOMAIN_VIRT_XEN ?
> + VIR_DOMAIN_MEMBALLOON_MODEL_XEN :
> + VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO;
> + def->memballoon = memballoon;
Yeah, someone else may disagree, but I think all of this should be in
the hypervisor. The parser should only translate what's in the XML
directly into the config object without trying to infer things into it
based on the hypervisor type or arch.
I realize that you're just modifying similar code that was already
there, but we've been trying to move this type of code out of the parser
rather than add more of it.
> }
>
> /* Parse the RNG device */
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-arm-vexpressa9-nodevs.args b/tests/qemuxml2argvdata/qemuxml2argv-arm-vexpressa9-nodevs.args
> new file mode 100644
> index 0000000..129f2ed
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-arm-vexpressa9-nodevs.args
> @@ -0,0 +1 @@
> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu-system-arm -S -M vexpress-a9 -m 1024 -smp 1 -nographic -nodefconfig -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -boot c -kernel /arm-kernel -initrd /arm-initrd -append console=ttyAMA0,115200n8 -dtb /arm-dtb
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-arm-vexpressa9-nodevs.xml b/tests/qemuxml2argvdata/qemuxml2argv-arm-vexpressa9-nodevs.xml
> new file mode 100644
> index 0000000..6a97e98
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-arm-vexpressa9-nodevs.xml
> @@ -0,0 +1,26 @@
> +<domain type="qemu">
> + <name>armtest</name>
> + <uuid>496d7ea8-9739-544b-4ebd-ef08be936e6a</uuid>
> + <memory>1048576</memory>
> + <currentMemory>1048576</currentMemory>
> + <vcpu>1</vcpu>
> + <os>
> + <type arch="armv7l" machine="vexpress-a9">hvm</type>
> + <kernel>/arm-kernel</kernel>
> + <initrd>/arm-initrd</initrd>
> + <dtb>/arm-dtb</dtb>
> + <cmdline>console=ttyAMA0,115200n8</cmdline>
> + </os>
> + <features>
> + <acpi/>
> + <apic/>
> + <pae/>
> + </features>
> + <clock offset="utc"/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>restart</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-arm</emulator>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index b7485fc..361ddb8 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1027,6 +1027,9 @@ mymain(void)
> DO_TEST_PARSE_ERROR("pci-root-address",
> QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE);
>
> + DO_TEST("arm-vexpressa9-nodevs",
> + QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DTB);
> +
> virObjectUnref(driver.config);
> virObjectUnref(driver.caps);
> virObjectUnref(driver.xmlopt);
> diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
> index fac83b2..3907683 100644
> --- a/tests/testutilsqemu.c
> +++ b/tests/testutilsqemu.c
> @@ -145,6 +145,35 @@ error:
> return -1;
> }
>
> +static int testQemuAddArmGuest(virCapsPtr caps)
> +{
> + static const char *machines[] = { "vexpress-a9",
> + "versatilepb" };
> + virCapsGuestMachinePtr *capsmachines = NULL;
> + virCapsGuestPtr guest;
> +
> + capsmachines = virCapabilitiesAllocMachines(machines,
> + ARRAY_CARDINALITY(machines));
> + if (!capsmachines)
> + goto error;
> +
> + guest = virCapabilitiesAddGuest(caps, "hvm", VIR_ARCH_ARMV7L,
> + "/usr/bin/qemu-system-arm", NULL,
> + ARRAY_CARDINALITY(machines),
> + capsmachines);
> + if (!guest)
> + goto error;
> +
> + if (!virCapabilitiesAddGuestDomain(guest, "qemu", NULL, NULL, 0, NULL))
> + goto error;
> +
> + return 0;
> +
> +error:
> + virCapabilitiesFreeMachines(capsmachines, ARRAY_CARDINALITY(machines));
> + return -1;
> +}
> +
>
> virCapsPtr testQemuCapsInit(void) {
> virCapsPtr caps;
> @@ -270,6 +299,9 @@ virCapsPtr testQemuCapsInit(void) {
> if (testQemuAddS390Guest(caps))
> goto cleanup;
>
> + if (testQemuAddArmGuest(caps))
> + goto cleanup;
> +
> if (virTestGetDebug()) {
> char *caps_str;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list