[libvirt] [PATCH 2/4] conf: Allow UEFI NVRAM format selection
Laszlo Ersek
lersek at redhat.com
Tue Jan 13 14:59:02 UTC 2015
comments below
On 01/13/15 14:41, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1180955
>
> Up till now, there was only one format for NVRAM store file.
> However, it's possible to convert the file to qcow2 (which
> supports interesting features, like snapshotting, for instance).
> Therefore, we want to allow format selection over the 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
> <nvram/> element.
>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
> docs/formatdomain.html.in | 8 +++--
> docs/schemas/domaincommon.rng | 8 +++++
> src/conf/domain_conf.c | 40 +++++++++++++++++-----
> src/conf/domain_conf.h | 10 ++++++
> src/libvirt_private.syms | 2 ++
> .../qemuxml2xmlout-bios-nvram.xml | 40 ++++++++++++++++++++++
> tests/qemuxml2xmltest.c | 2 +-
> 7 files changed, 98 insertions(+), 12 deletions(-)
> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-bios-nvram.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 0c3343e..167114a 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -103,7 +103,7 @@
> <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>
> + <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'/>
> @@ -150,7 +150,11 @@
> 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
You're mid-sentence; I think it should be "since", not "Since".
Alternatively, drop "Then, ".
> + the NVRAM file format: <code>raw</code> for raw NVRAM file (the
> + default) or <code>qcow2</code> if the file is converted to qcow2
> + format.</dd>
s/is converted to/has/. We shouldn't imply any activity wrt. to the
conversion.
> <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..0afd02a 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -269,6 +269,14 @@
> </attribute>
> </optional>
> <optional>
> + <attribute name="format">
> + <choice>
> + <value>raw</value>
> + <value>qcow2</value>
> + </choice>
> + </attribute>
> + </optional>
> + <optional>
> <ref name="absFilePath"/>
> </optional>
> </element>
Okay, since we seem to be past the specification part, some points to
consider:
- the format specified for <nvram> affects the nvram template the same
way (which is given, if it is given, in another attribute of <nvram>). I
think no extra code should be necessary here, but maybe the docs should
mention it somewhere. (Just an idea to consider.) The "instantiation
from the template" logic doesn't care, so that's fine.
- Don't forget the unified (non-split) case, when you have no <nvram>
element at all, but you have a *read-write* pflash *loader*. (Containing
both executable code & the varstore.) You need a format specification
for the loader too. (On the qemu level, it's (almost) just another
pflash drive, so it has a format, and some users might want to use a
unified OVMF.fd, without separate nvram, template etc.)
This use case is only important for:
<loader readonly='no' type='pflash'>.../OVMF.fd</loader>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3cbb93d..d3b6378 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(virDomainLoaderNVRAMFormat,
> + VIR_DOMAIN_LOADER_NVRAM_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)
> @@ -12617,16 +12622,18 @@ 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;
> + char *format_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,10 +12652,24 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
> loader->type = type;
> }
>
> + if (nvram_node) {
> + loader->nvram = (char *)xmlNodeGetContent(nvram_node);
> + loader->templt = virXMLPropString(nvram_node, "template");
> + format_str = virXMLPropString(nvram_node, "format");
> +
> + if (format_str &&
> + (loader->nvramFormat = virDomainLoaderNVRAMFormatTypeFromString(format_str)) < 0) {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("unknown format value: %s"), format_str);
not sure if this reports detailed location info; I guess you could
mention <nvram>. (Again, not sure about the tradition here.)
> + goto cleanup;
> + }
> + }
> +
I'm confused. Aside from our wish to add "format" to <loader> as well,
ie. accepting for a minute that only <nvram> gets the new attribute:
why are we populating loader->nvram and loader->templt here? These
changes seem to be unrelated to our goal.
> ret = 0;
> cleanup:
> VIR_FREE(readonly_str);
> VIR_FREE(type_str);
> + VIR_FREE(format_str);
> return ret;
> }
>
> @@ -13735,7 +13756,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 +13767,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;
> }
> }
I guess this simplification makes sense (for loader->nvram and
loader->templt), but, if you need it, could you extract this change into
a separate patch?
>
> @@ -19288,6 +19308,8 @@ virDomainLoaderDefFormat(virBufferPtr buf,
> if (loader->nvram || loader->templt) {
> virBufferAddLit(buf, "<nvram");
> virBufferEscapeString(buf, " template='%s'", loader->templt);
> + virBufferAsprintf(buf, " format='%s'",
> + virDomainLoaderNVRAMFormatTypeToString(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..428afcc 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1729,6 +1729,15 @@ typedef enum {
>
> VIR_ENUM_DECL(virDomainLoader)
>
> +typedef enum {
> + VIR_DOMAIN_LOADER_NVRAM_FORMAT_RAW = 0,
> + VIR_DOMAIN_LOADER_NVRAM_FORMAT_QCOW2,
> +
> + VIR_DOMAIN_LOADER_NVRAM_FORMAT_LAST
> +} virDomainLoaderNVRAMFormat;
> +
> +VIR_ENUM_DECL(virDomainLoaderNVRAMFormat)
> +
> typedef struct _virDomainLoaderDef virDomainLoaderDef;
> typedef virDomainLoaderDef *virDomainLoaderDefPtr;
> struct _virDomainLoaderDef {
> @@ -1737,6 +1746,7 @@ struct _virDomainLoaderDef {
> virDomainLoader type;
> char *nvram; /* path to non-volatile RAM */
> char *templt; /* user override of path to master nvram */
> + virDomainLoaderNVRAMFormat nvramFormat;
> };
>
> void virDomainLoaderDefFree(virDomainLoaderDefPtr loader);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index fb5d003..2dd73db 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -316,6 +316,8 @@ virDomainLifecycleTypeToString;
> virDomainListFree;
> virDomainLiveConfigHelperMethod;
> virDomainLoaderDefFree;
> +virDomainLoaderNVRAMFormatTypeFromString;
> +virDomainLoaderNVRAMFormatTypeToString;
> 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..f622e1c
> --- /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'>/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");
>
I hope I didn't miss anything.
Thanks
Laszlo
More information about the libvir-list
mailing list