[libvirt] [PATCH v2 0/4] UEFI and NVRAM store file format
diff to v1: -Laszlo's review worked in (notably, even UEFI can have a different format than raw) Michal Privoznik (4): conf: Make virDomainLoaderDefParseXML parse <nvram/> too conf: Allow UEFI and NVRAM format selection qemu: Adapt to UEFI and NVRAM store file format change domaincaps: Expose UEFI and NVRAM store file format capabilities docs/formatdomain.html.in | 18 +++++-- docs/formatdomaincaps.html.in | 26 ++++++++++ docs/schemas/domaincaps.rng | 13 +++++ docs/schemas/domaincommon.rng | 16 ++++++ src/conf/domain_capabilities.c | 18 +++++++ src/conf/domain_capabilities.h | 2 + src/conf/domain_conf.c | 57 +++++++++++++++++----- src/conf/domain_conf.h | 11 +++++ src/libvirt_private.syms | 2 + src/qemu/qemu_capabilities.c | 10 +++- src/qemu/qemu_command.c | 12 +++-- tests/domaincapsschemadata/domaincaps-full.xml | 10 ++++ .../domaincaps-qemu_1.6.50-1.xml | 10 ++++ tests/domaincapstest.c | 2 + .../qemuxml2argv-bios-nvram-qcow2.args | 10 ++++ .../qemuxml2argv-bios-nvram-qcow2.xml | 40 +++++++++++++++ tests/qemuxml2argvtest.c | 2 + .../qemuxml2xmlout-bios-nvram.xml | 40 +++++++++++++++ tests/qemuxml2xmltest.c | 2 +- 19 files changed, 279 insertions(+), 22 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram-qcow2.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram-qcow2.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-bios-nvram.xml -- 2.0.5
This is pure code movement without any functional change. The code simply reads better this way, that's all. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3cbb93d..ae18255 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12617,16 +12617,17 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) } static int -virDomainLoaderDefParseXML(xmlNodePtr node, +virDomainLoaderDefParseXML(xmlNodePtr loader_node, + xmlNodePtr nvram_node, virDomainLoaderDefPtr loader) { int ret = -1; char *readonly_str = NULL; char *type_str = NULL; - readonly_str = virXMLPropString(node, "readonly"); - type_str = virXMLPropString(node, "type"); - loader->path = (char *) xmlNodeGetContent(node); + readonly_str = virXMLPropString(loader_node, "readonly"); + type_str = virXMLPropString(loader_node, "type"); + loader->path = (char *) xmlNodeGetContent(loader_node); if (readonly_str && (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) { @@ -12645,6 +12646,11 @@ virDomainLoaderDefParseXML(xmlNodePtr node, loader->type = type; } + if (nvram_node) { + loader->nvram = (char *)xmlNodeGetContent(nvram_node); + loader->templt = virXMLPropString(nvram_node, "template"); + } + ret = 0; cleanup: VIR_FREE(readonly_str); @@ -13735,7 +13741,7 @@ virDomainDefParseXML(xmlDocPtr xml, if (STREQ(def->os.type, "xen") || STREQ(def->os.type, "hvm") || STREQ(def->os.type, "uml")) { - xmlNodePtr loader_node; + xmlNodePtr loader_node, nvram_node; def->os.kernel = virXPathString("string(./os/kernel[1])", ctxt); def->os.initrd = virXPathString("string(./os/initrd[1])", ctxt); @@ -13746,11 +13752,10 @@ virDomainDefParseXML(xmlDocPtr xml, if (VIR_ALLOC(def->os.loader) < 0) goto error; - if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0) - goto error; + nvram_node = virXPathNode("./os/nvram[1]", ctxt); - def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); - def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt); + if (virDomainLoaderDefParseXML(loader_node, nvram_node, def->os.loader) < 0) + goto error; } } -- 2.0.5
On 01/13/15 18:20, Michal Privoznik wrote:
This is pure code movement without any functional change. The code simply reads better this way, that's all.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3cbb93d..ae18255 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12617,16 +12617,17 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) }
static int -virDomainLoaderDefParseXML(xmlNodePtr node, +virDomainLoaderDefParseXML(xmlNodePtr loader_node, + xmlNodePtr nvram_node, virDomainLoaderDefPtr loader) { int ret = -1; char *readonly_str = NULL; char *type_str = NULL;
- readonly_str = virXMLPropString(node, "readonly"); - type_str = virXMLPropString(node, "type"); - loader->path = (char *) xmlNodeGetContent(node); + readonly_str = virXMLPropString(loader_node, "readonly"); + type_str = virXMLPropString(loader_node, "type"); + loader->path = (char *) xmlNodeGetContent(loader_node);
if (readonly_str && (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) { @@ -12645,6 +12646,11 @@ virDomainLoaderDefParseXML(xmlNodePtr node, loader->type = type; }
+ if (nvram_node) { + loader->nvram = (char *)xmlNodeGetContent(nvram_node); + loader->templt = virXMLPropString(nvram_node, "template"); + } + ret = 0; cleanup: VIR_FREE(readonly_str); @@ -13735,7 +13741,7 @@ virDomainDefParseXML(xmlDocPtr xml, if (STREQ(def->os.type, "xen") || STREQ(def->os.type, "hvm") || STREQ(def->os.type, "uml")) { - xmlNodePtr loader_node; + xmlNodePtr loader_node, nvram_node;
def->os.kernel = virXPathString("string(./os/kernel[1])", ctxt); def->os.initrd = virXPathString("string(./os/initrd[1])", ctxt); @@ -13746,11 +13752,10 @@ virDomainDefParseXML(xmlDocPtr xml, if (VIR_ALLOC(def->os.loader) < 0) goto error;
- if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0) - goto error; + nvram_node = virXPathNode("./os/nvram[1]", ctxt);
- def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); - def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt); + if (virDomainLoaderDefParseXML(loader_node, nvram_node, def->os.loader) < 0) + goto error; } }
You turned the two invocations of virXPathString() into a call to xmlNodeGetContent() and another to virXMLPropString(). I tried to verify if the cases when the former function returns NULL are identical to the cases when the latter functions return NULL. I'm not 100% sure, but quite. In particular, virXPathString() returns NULL for an empty XPath string (see (obj->stringval[0] == 0) in it), which quite explains the string() conversion inside the XPath expression (pre-patch). Post-patch, I'm unsure. The nvram_node assignment looks okay I guess. The other two calls: - http://xmlsoft.org/html/libxml-tree.html#xmlNodeGetContent - http://xmlsoft.org/html/libxml-tree.html#xmlGetProp (wrapped by virXMLPropString()) are dubious, especially for the template assignment. Pre-patch, an empty string in the XML stored a NULL, post-patch, I'm unsure if an empty string in the XML can return "" rather than NULL. I can ACK this if you clean up my doubts :) (You can justify the code simply by testing empty content and empty attribute value too.) Thanks Laszlo
On 01/13/15 19:47, Laszlo Ersek wrote:
On 01/13/15 18:20, Michal Privoznik wrote:
This is pure code movement without any functional change. The code simply reads better this way, that's all.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3cbb93d..ae18255 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12617,16 +12617,17 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) }
static int -virDomainLoaderDefParseXML(xmlNodePtr node, +virDomainLoaderDefParseXML(xmlNodePtr loader_node, + xmlNodePtr nvram_node, virDomainLoaderDefPtr loader) { int ret = -1; char *readonly_str = NULL; char *type_str = NULL;
- readonly_str = virXMLPropString(node, "readonly"); - type_str = virXMLPropString(node, "type"); - loader->path = (char *) xmlNodeGetContent(node); + readonly_str = virXMLPropString(loader_node, "readonly"); + type_str = virXMLPropString(loader_node, "type"); + loader->path = (char *) xmlNodeGetContent(loader_node);
if (readonly_str && (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) { @@ -12645,6 +12646,11 @@ virDomainLoaderDefParseXML(xmlNodePtr node, loader->type = type; }
+ if (nvram_node) { + loader->nvram = (char *)xmlNodeGetContent(nvram_node); + loader->templt = virXMLPropString(nvram_node, "template"); + } + ret = 0; cleanup: VIR_FREE(readonly_str); @@ -13735,7 +13741,7 @@ virDomainDefParseXML(xmlDocPtr xml, if (STREQ(def->os.type, "xen") || STREQ(def->os.type, "hvm") || STREQ(def->os.type, "uml")) { - xmlNodePtr loader_node; + xmlNodePtr loader_node, nvram_node;
def->os.kernel = virXPathString("string(./os/kernel[1])", ctxt); def->os.initrd = virXPathString("string(./os/initrd[1])", ctxt); @@ -13746,11 +13752,10 @@ virDomainDefParseXML(xmlDocPtr xml, if (VIR_ALLOC(def->os.loader) < 0) goto error;
- if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0) - goto error; + nvram_node = virXPathNode("./os/nvram[1]", ctxt);
- def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); - def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt); + if (virDomainLoaderDefParseXML(loader_node, nvram_node, def->os.loader) < 0) + goto error; } }
You turned the two invocations of virXPathString() into a call to xmlNodeGetContent() and another to virXMLPropString(). I tried to verify if the cases when the former function returns NULL are identical to the cases when the latter functions return NULL. I'm not 100% sure, but quite.
In particular, virXPathString() returns NULL for an empty XPath string (see (obj->stringval[0] == 0) in it), which quite explains the string() conversion inside the XPath expression (pre-patch).
Post-patch, I'm unsure. The nvram_node assignment looks okay I guess.
The other two calls: - http://xmlsoft.org/html/libxml-tree.html#xmlNodeGetContent - http://xmlsoft.org/html/libxml-tree.html#xmlGetProp (wrapped by virXMLPropString())
are dubious, especially for the template assignment. Pre-patch, an empty string in the XML stored a NULL, post-patch, I'm unsure if an empty string in the XML can return "" rather than NULL.
I can ACK this if you clean up my doubts :)
(You can justify the code simply by testing empty content and empty attribute value too.)
Actually, thinking more on it -- the patch cannot introduce more NULLs, it can introduce only more empty strings. Ie. it can't introduce any crashes; at worst it can introduce empty filenames (for empty node contents and empty attribute values) which will then lead to file not found / unable to create file errors. Specifying an nvram file and/or a template with the empty string name never made much sense, so it doesn't matter if the behavior changes. (I'm unsure if it changes, but even if it does, it's okay.) Acked-by: Laszlo Ersek <lersek@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1180955 Up till now, there was only one format for UEFI and NVRAM store file. However, it's possible to convert the files to qcow2 (which supports interesting features, like snapshotting, for instance). Therefore, we want to allow format selection over the UEFI and NVRAM store file. By default, the format is raw, but users can choose qcow2. In the domain XML this is expressed as @format attribute to <loader/> and <nvram/> elements. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 18 +++++++--- docs/schemas/domaincommon.rng | 16 +++++++++ src/conf/domain_conf.c | 34 ++++++++++++++++-- src/conf/domain_conf.h | 11 ++++++ src/libvirt_private.syms | 2 ++ .../qemuxml2xmlout-bios-nvram.xml | 40 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 2 +- 7 files changed, 116 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-bios-nvram.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0c3343e..12150f2 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -102,8 +102,8 @@ ... <os> <type>hvm</type> - <loader readonly='yes' type='rom'>/usr/lib/xen/boot/hvmloader</loader> - <nvram template='/usr/share/OVMF/OVMF_VARS.fd'>/var/lib/libvirt/nvram/guest_VARS.fd</nvram> + <loader readonly='yes' type='rom' format='raw'>/usr/lib/xen/boot/hvmloader</loader> + <nvram template='/usr/share/OVMF/OVMF_VARS.fd' format='raw'>/var/lib/libvirt/nvram/guest_VARS.fd</nvram> <boot dev='hd'/> <boot dev='cdrom'/> <bootmenu enable='yes' timeout='3000'/> @@ -139,7 +139,12 @@ <code>pflash</code>. It tells the hypervisor where in the guest memory the file should be mapped. For instance, if the loader path points to an UEFI image, <code>type</code> should be - <code>pflash</code>.</dd> + <code>pflash</code>.<span class="since">Since 1.2.12</span> it's + possible to determine the loader format too. However, other values than + <code>raw</code> make sense only if the UEFI image file is writable + (that is, the image and variable store is not split), that is when + <code>readonly</code> is no. Accepted values are <code>raw</code> + which is the default or <code>qcow2</code>.</dd> <dt><code>nvram</code></dt> <dd>Some UEFI firmwares may want to use a non-volatile memory to store some variables. In the host, this is represented as a file and the path @@ -150,7 +155,12 @@ from the config file. Note, that for transient domains if the NVRAM file has been created by libvirt it is left behind and it is management application's responsibility to save and remove file (if needed to be - persistent). <span class="since">Since 1.2.8</span></dd> + persistent). <span class="since">Since 1.2.8</span> + Then, <span class="since">since 1.2.12</span> it's possible to choose + the NVRAM file format: <code>raw</code> for raw NVRAM file (the + default) or <code>qcow2</code> if the file has qcow2 format. Note that + if used, the template file must be in the same format. Libvirt does no + format conversion.</dd> <dt><code>boot</code></dt> <dd>The <code>dev</code> attribute takes one of the values "fd", "hd", "cdrom" or "network" and is used to specify the next boot device diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 85b3709..0010557 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -258,6 +258,14 @@ </choice> </attribute> </optional> + <optional> + <attribute name="format"> + <choice> + <value>raw</value> + <value>qcow2</value> + </choice> + </attribute> + </optional> <ref name="absFilePath"/> </element> </optional> @@ -269,6 +277,14 @@ </attribute> </optional> <optional> + <attribute name="format"> + <choice> + <value>raw</value> + <value>qcow2</value> + </choice> + </attribute> + </optional> + <optional> <ref name="absFilePath"/> </optional> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ae18255..bc23c6c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -794,6 +794,11 @@ VIR_ENUM_IMPL(virDomainLoader, "rom", "pflash") +VIR_ENUM_IMPL(virDomainLoaderFormat, + VIR_DOMAIN_LOADER_FORMAT_LAST, + "raw", + "qcow2") + /* Internal mapping: subset of block job types that can be present in * <mirror> XML (remaining types are not two-phase). */ VIR_ENUM_DECL(virDomainBlockJob) @@ -12624,9 +12629,11 @@ virDomainLoaderDefParseXML(xmlNodePtr loader_node, int ret = -1; char *readonly_str = NULL; char *type_str = NULL; + char *format_str = NULL; readonly_str = virXMLPropString(loader_node, "readonly"); type_str = virXMLPropString(loader_node, "type"); + format_str = virXMLPropString(loader_node, "format"); loader->path = (char *) xmlNodeGetContent(loader_node); if (readonly_str && @@ -12646,15 +12653,33 @@ virDomainLoaderDefParseXML(xmlNodePtr loader_node, loader->type = type; } + if (format_str && loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH && + (loader->format = virDomainLoaderFormatTypeFromString(format_str)) < 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("unknown format value to <loader/>: %s"), + format_str); + goto cleanup; + } + if (nvram_node) { + VIR_FREE(format_str); loader->nvram = (char *)xmlNodeGetContent(nvram_node); loader->templt = virXMLPropString(nvram_node, "template"); + format_str = virXMLPropString(nvram_node, "format"); + + if (format_str && + (loader->nvramFormat = virDomainLoaderFormatTypeFromString(format_str)) < 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("unknown format value to <nvram/>: %s"), format_str); + goto cleanup; + } } ret = 0; cleanup: VIR_FREE(readonly_str); VIR_FREE(type_str); + VIR_FREE(format_str); return ret; } @@ -19287,12 +19312,17 @@ virDomainLoaderDefFormat(virBufferPtr buf, if (loader->readonly) virBufferAsprintf(buf, " readonly='%s'", readonly); - virBufferAsprintf(buf, " type='%s'>", type); + virBufferAsprintf(buf, " type='%s'", type); + if (loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH) + virBufferAsprintf(buf, " format='%s'", + virDomainLoaderFormatTypeToString(loader->format)); - virBufferEscapeString(buf, "%s</loader>\n", loader->path); + virBufferEscapeString(buf, ">%s</loader>\n", loader->path); if (loader->nvram || loader->templt) { virBufferAddLit(buf, "<nvram"); virBufferEscapeString(buf, " template='%s'", loader->templt); + virBufferAsprintf(buf, " format='%s'", + virDomainLoaderFormatTypeToString(loader->nvramFormat)); if (loader->nvram) virBufferEscapeString(buf, ">%s</nvram>\n", loader->nvram); else diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ac1f4f8..7ea7714 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1729,14 +1729,25 @@ typedef enum { VIR_ENUM_DECL(virDomainLoader) +typedef enum { + VIR_DOMAIN_LOADER_FORMAT_RAW = 0, + VIR_DOMAIN_LOADER_FORMAT_QCOW2, + + VIR_DOMAIN_LOADER_FORMAT_LAST +} virDomainLoaderFormat; + +VIR_ENUM_DECL(virDomainLoaderFormat) + typedef struct _virDomainLoaderDef virDomainLoaderDef; typedef virDomainLoaderDef *virDomainLoaderDefPtr; struct _virDomainLoaderDef { char *path; int readonly; /* enum virTristateBool */ virDomainLoader type; + virDomainLoaderFormat format; char *nvram; /* path to non-volatile RAM */ char *templt; /* user override of path to master nvram */ + virDomainLoaderFormat nvramFormat; }; void virDomainLoaderDefFree(virDomainLoaderDefPtr loader); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fb5d003..5db851c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -316,6 +316,8 @@ virDomainLifecycleTypeToString; virDomainListFree; virDomainLiveConfigHelperMethod; virDomainLoaderDefFree; +virDomainLoaderFormatTypeFromString; +virDomainLoaderFormatTypeToString; virDomainLoaderTypeFromString; virDomainLoaderTypeToString; virDomainLockFailureTypeFromString; diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-bios-nvram.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-bios-nvram.xml new file mode 100644 index 0000000..d29df38 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-bios-nvram.xml @@ -0,0 +1,40 @@ +<domain type='qemu'> + <name>test-bios</name> + <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <loader readonly='yes' type='pflash' format='raw'>/usr/share/OVMF/OVMF_CODE.fd</loader> + <nvram format='raw'>/usr/share/OVMF/OVMF_VARS.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <input type='tablet' bus='usb'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 1166534..5238b09 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -404,7 +404,7 @@ mymain(void) DO_TEST_DIFFERENT("numatune-memnode"); DO_TEST("numatune-memnode-no-memory"); - DO_TEST("bios-nvram"); + DO_TEST_DIFFERENT("bios-nvram"); DO_TEST("tap-vhost"); DO_TEST("shmem"); -- 2.0.5
On 01/13/15 18:20, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1180955
Up till now, there was only one format for UEFI and NVRAM store file. However, it's possible to convert the files to qcow2 (which supports interesting features, like snapshotting, for instance). Therefore, we want to allow format selection over the UEFI and NVRAM store file. By default, the format is raw, but users can choose qcow2. In the domain XML this is expressed as @format attribute to <loader/> and <nvram/> elements.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 18 +++++++--- docs/schemas/domaincommon.rng | 16 +++++++++ src/conf/domain_conf.c | 34 ++++++++++++++++-- src/conf/domain_conf.h | 11 ++++++ src/libvirt_private.syms | 2 ++ .../qemuxml2xmlout-bios-nvram.xml | 40 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 2 +- 7 files changed, 116 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-bios-nvram.xml
Looks good to me. Acked-by: Laszlo Ersek <lersek@redhat.com>
This basically implements the availability of choosing UEFI and NVRAM store file format in the qemu driver. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 12 ++++--- .../qemuxml2argv-bios-nvram-qcow2.args | 10 ++++++ .../qemuxml2argv-bios-nvram-qcow2.xml | 40 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 4 files changed, 60 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram-qcow2.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram-qcow2.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3346e95..44fa331 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7686,8 +7686,10 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd, } virBufferAsprintf(&buf, - "file=%s,if=pflash,format=raw,unit=%d", - loader->path, unit); + "file=%s,if=pflash,format=%s,unit=%d", + loader->path, + virDomainLoaderFormatTypeToString(loader->format), + unit); unit++; if (loader->readonly) { @@ -7708,8 +7710,10 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd, if (loader->nvram) { virBufferFreeAndReset(&buf); virBufferAsprintf(&buf, - "file=%s,if=pflash,format=raw,unit=%d", - loader->nvram, unit); + "file=%s,if=pflash,format=%s,unit=%d", + loader->nvram, + virDomainLoaderFormatTypeToString(loader->nvramFormat), + unit); virCommandAddArg(cmd, "-drive"); virCommandAddArgBuffer(cmd, &buf); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram-qcow2.args b/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram-qcow2.args new file mode 100644 index 0000000..b9575e4 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram-qcow2.args @@ -0,0 +1,10 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc \ +-drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \ +-drive file=/usr/share/OVMF/OVMF_VARS.qcow2,if=pflash,format=qcow2,unit=1 \ +-m 1024 -smp 1 -nographic -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -boot c -usb \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-ide0-0-0,format=raw \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-serial pty -device usb-tablet,id=input0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram-qcow2.xml b/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram-qcow2.xml new file mode 100644 index 0000000..25870f4 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram-qcow2.xml @@ -0,0 +1,40 @@ +<domain type='qemu'> + <name>test-bios</name> + <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> + <nvram format='qcow2'>/usr/share/OVMF/OVMF_VARS.qcow2</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <input type='tablet' bus='usb'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1d0bd61..86b3d0e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -665,6 +665,8 @@ mymain(void) DO_TEST("bios", QEMU_CAPS_DEVICE, QEMU_CAPS_SGA); DO_TEST("bios-nvram", QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT, QEMU_CAPS_DRIVE_READONLY); + DO_TEST("bios-nvram-qcow2", QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, + QEMU_CAPS_DRIVE_FORMAT, QEMU_CAPS_DRIVE_READONLY); DO_TEST("clock-utc", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE); DO_TEST("clock-localtime", NONE); DO_TEST("clock-localtime-basis-localtime", QEMU_CAPS_RTC); -- 2.0.5
On 01/13/15 18:20, Michal Privoznik wrote:
This basically implements the availability of choosing UEFI and NVRAM store file format in the qemu driver.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 12 ++++--- .../qemuxml2argv-bios-nvram-qcow2.args | 10 ++++++ .../qemuxml2argv-bios-nvram-qcow2.xml | 40 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 4 files changed, 60 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram-qcow2.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram-qcow2.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3346e95..44fa331 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7686,8 +7686,10 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd, }
virBufferAsprintf(&buf, - "file=%s,if=pflash,format=raw,unit=%d", - loader->path, unit); + "file=%s,if=pflash,format=%s,unit=%d", + loader->path, + virDomainLoaderFormatTypeToString(loader->format), + unit); unit++;
if (loader->readonly) { @@ -7708,8 +7710,10 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd, if (loader->nvram) { virBufferFreeAndReset(&buf); virBufferAsprintf(&buf, - "file=%s,if=pflash,format=raw,unit=%d", - loader->nvram, unit); + "file=%s,if=pflash,format=%s,unit=%d", + loader->nvram, + virDomainLoaderFormatTypeToString(loader->nvramFormat), + unit);
virCommandAddArg(cmd, "-drive"); virCommandAddArgBuffer(cmd, &buf);
Acked-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo
It would be nice if we expose the capability of choosing the UEFI and NVRAM store file format among with all the possibilities. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomaincaps.html.in | 26 ++++++++++++++++++++++ docs/schemas/domaincaps.rng | 13 +++++++++++ src/conf/domain_capabilities.c | 18 +++++++++++++++ src/conf/domain_capabilities.h | 2 ++ src/qemu/qemu_capabilities.c | 10 +++++++-- tests/domaincapsschemadata/domaincaps-full.xml | 10 +++++++++ .../domaincaps-qemu_1.6.50-1.xml | 10 +++++++++ tests/domaincapstest.c | 2 ++ 8 files changed, 89 insertions(+), 2 deletions(-) diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in index 850109f..10e382f 100644 --- a/docs/formatdomaincaps.html.in +++ b/docs/formatdomaincaps.html.in @@ -117,7 +117,17 @@ <value>yes</value> <value>no</value> </enum> + <enum name='format'> + <value>raw</value> + <value>qcow2</value> + </enum> </loader> + <nvram supported='yes'> + <enum name='format'> + <value>raw</value> + <value>qcow2</value> + </enum> + </nvram> </os> ... <domainCapabilities> @@ -140,6 +150,22 @@ <dt>readonly</dt> <dd>Options for the <code>readonly</code> attribute of the <loader/> element.</dd> + + <dt>format</dt> + <dd>What formats of UEFI file are supported. The default is + <code>raw</code>, but depending on hypervisor's capabilities, + <code>qcow2</code> may be supported as well. This refers to + <code>format</code> attribute of the <loader/> element.</dd> + </dl> + + <p>For the <code>nvram</code> element, the following can be listed:</p> + + <dl> + <dt>format</dt> + <dd>What formats of NVRAM store file are supported. The default is + <code>raw</code>, but depending on hypervisor's capabilities, + <code>qcow2</code> may be supported as well. This refers to + <code>format</code> attribute of the <nvram/> element.</dd> </dl> <h3><a name="elementsDevices">Devices</a></h3> diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng index 35d3745..e8a1e59 100644 --- a/docs/schemas/domaincaps.rng +++ b/docs/schemas/domaincaps.rng @@ -54,6 +54,16 @@ </element> </define> + <define name='nvram'> + <element name='nvram'> + <ref name='supported'/> + <optional> + <ref name='value'/> + </optional> + <ref name='enum'/> + </element> + </define> + <define name='os'> <element name='os'> <interleave> @@ -61,6 +71,9 @@ <optional> <ref name='loader'/> </optional> + <optional> + <ref name='nvram'/> + </optional> </interleave> </element> </define> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 7c59912..9393d06 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -216,11 +216,28 @@ virDomainCapsLoaderFormat(virBufferPtr buf, virDomainCapsStringValuesFormat(buf, &loader->values); ENUM_PROCESS(loader, type, virDomainLoaderTypeToString); ENUM_PROCESS(loader, readonly, virTristateBoolTypeToString); + ENUM_PROCESS(loader, format, virDomainLoaderFormatTypeToString); FORMAT_EPILOGUE(loader); } static void +virDomainCapsNVRAMFormat(virBufferPtr buf, + virDomainCapsLoaderPtr nvram) +{ + /* Even though the type of @nvram would suggest naming it + * differently than 'nvram' ('loader' for instance), keep in + * mind that the FORMAT_PROLOGUE macro emits the variable + * name into the XML where we want to see 'nvram'. Really. */ + FORMAT_PROLOGUE(nvram); + + virDomainCapsEnumFormat(buf, &nvram->nvramFormat, "format", + virDomainLoaderFormatTypeToString); + + FORMAT_EPILOGUE(nvram); +} + +static void virDomainCapsOSFormat(virBufferPtr buf, virDomainCapsOSPtr os) { @@ -229,6 +246,7 @@ virDomainCapsOSFormat(virBufferPtr buf, FORMAT_PROLOGUE(os); virDomainCapsLoaderFormat(buf, loader); + virDomainCapsNVRAMFormat(buf, loader); FORMAT_EPILOGUE(os); } diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 597ac75..d59b126 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -57,6 +57,8 @@ struct _virDomainCapsLoader { virDomainCapsStringValues values; /* Info about values for the element */ virDomainCapsEnum type; /* Info about virDomainLoader */ virDomainCapsEnum readonly; /* Info about readonly:virTristateBool */ + virDomainCapsEnum format; /* Info about virDomainLoaderFormat */ + virDomainCapsEnum nvramFormat; /* Info about virDomainLoaderFormat */ }; typedef struct _virDomainCapsOS virDomainCapsOS; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 13f3cd3..addd9d3 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3706,10 +3706,16 @@ virQEMUCapsFillDomainLoaderCaps(virQEMUCapsPtr qemuCaps, VIR_DOMAIN_LOADER_TYPE_ROM); if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_FORMAT)) + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_FORMAT)) { VIR_DOMAIN_CAPS_ENUM_SET(capsLoader->type, VIR_DOMAIN_LOADER_TYPE_PFLASH); - + VIR_DOMAIN_CAPS_ENUM_SET(capsLoader->format, + VIR_DOMAIN_LOADER_FORMAT_RAW, + VIR_DOMAIN_LOADER_FORMAT_QCOW2); + VIR_DOMAIN_CAPS_ENUM_SET(capsLoader->nvramFormat, + VIR_DOMAIN_LOADER_FORMAT_RAW, + VIR_DOMAIN_LOADER_FORMAT_QCOW2); + } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_READONLY)) VIR_DOMAIN_CAPS_ENUM_SET(capsLoader->readonly, diff --git a/tests/domaincapsschemadata/domaincaps-full.xml b/tests/domaincapsschemadata/domaincaps-full.xml index 96202bc..c2b501c 100644 --- a/tests/domaincapsschemadata/domaincaps-full.xml +++ b/tests/domaincapsschemadata/domaincaps-full.xml @@ -17,7 +17,17 @@ <value>yes</value> <value>no</value> </enum> + <enum name='format'> + <value>raw</value> + <value>qcow2</value> + </enum> </loader> + <nvram supported='yes'> + <enum name='format'> + <value>raw</value> + <value>qcow2</value> + </enum> + </nvram> </os> <devices> <disk supported='yes'> diff --git a/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml b/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml index 346ef65..d45d6b5 100644 --- a/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml +++ b/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml @@ -14,7 +14,17 @@ <value>yes</value> <value>no</value> </enum> + <enum name='format'> + <value>raw</value> + <value>qcow2</value> + </enum> </loader> + <nvram supported='yes'> + <enum name='format'> + <value>raw</value> + <value>qcow2</value> + </enum> + </nvram> </os> <devices> <disk supported='yes'> diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index 70d2ef3..b483de1 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -70,6 +70,8 @@ fillAll(virDomainCapsPtr domCaps, loader->device.supported = true; SET_ALL_BITS(loader->type); SET_ALL_BITS(loader->readonly); + SET_ALL_BITS(loader->format); + SET_ALL_BITS(loader->nvramFormat); if (fillStringValues(&loader->values, "/foo/bar", "/tmp/my_path", -- 2.0.5
On 01/13/15 18:20, Michal Privoznik wrote:
diff to v1: -Laszlo's review worked in (notably, even UEFI can have a different format than raw)
Michal Privoznik (4): conf: Make virDomainLoaderDefParseXML parse <nvram/> too conf: Allow UEFI and NVRAM format selection qemu: Adapt to UEFI and NVRAM store file format change domaincaps: Expose UEFI and NVRAM store file format capabilities
docs/formatdomain.html.in | 18 +++++-- docs/formatdomaincaps.html.in | 26 ++++++++++ docs/schemas/domaincaps.rng | 13 +++++ docs/schemas/domaincommon.rng | 16 ++++++ src/conf/domain_capabilities.c | 18 +++++++ src/conf/domain_capabilities.h | 2 + src/conf/domain_conf.c | 57 +++++++++++++++++----- src/conf/domain_conf.h | 11 +++++ src/libvirt_private.syms | 2 + src/qemu/qemu_capabilities.c | 10 +++- src/qemu/qemu_command.c | 12 +++-- tests/domaincapsschemadata/domaincaps-full.xml | 10 ++++ .../domaincaps-qemu_1.6.50-1.xml | 10 ++++ tests/domaincapstest.c | 2 + .../qemuxml2argv-bios-nvram-qcow2.args | 10 ++++ .../qemuxml2argv-bios-nvram-qcow2.xml | 40 +++++++++++++++ tests/qemuxml2argvtest.c | 2 + .../qemuxml2xmlout-bios-nvram.xml | 40 +++++++++++++++ tests/qemuxml2xmltest.c | 2 +- 19 files changed, 279 insertions(+), 22 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram-qcow2.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram-qcow2.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-bios-nvram.xml
Sigh. Please shelve this patch series for now. I'm very sorry for wasting your time. See bug 1180955 for details. :( Laszlo
participants (2)
-
Laszlo Ersek -
Michal Privoznik