This is going to extend virDomainLoader enum. The reason is that
once loader path is NULL its type makes no sense. However, since
value of zero corresponds to VIR_DOMAIN_LOADER_TYPE_ROM the
following XML would be produced:
<os>
<loader type='rom'/>
...
</os>
To solve this, introduce VIR_DOMAIN_LOADER_TYPE_NONE which would
correspond to value of zero and then use post parse callback to
set the default loader type to 'rom' if needed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/conf/domain_conf.c | 23 +++++++++++++++++++++--
src/conf/domain_conf.h | 3 ++-
src/qemu/qemu_command.c | 1 +
src/qemu/qemu_domain.c | 1 +
tests/domaincapsschemadata/full.xml | 1 +
5 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f622a4dddf..b436b91c66 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1013,6 +1013,7 @@ VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST,
VIR_ENUM_IMPL(virDomainLoader,
VIR_DOMAIN_LOADER_TYPE_LAST,
+ "none",
"rom",
"pflash",
);
@@ -4286,6 +4287,20 @@ virDomainDefPostParseMemory(virDomainDefPtr def,
}
+static void
+virDomainDefPostParseOs(virDomainDefPtr def)
+{
+ if (!def->os.loader)
+ return;
+
+ if (def->os.loader->path &&
+ def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_NONE) {
+ /* By default, loader is type of 'rom' */
+ def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
+ }
+}
+
+
static void
virDomainDefPostParseMemtune(virDomainDefPtr def)
{
@@ -5465,6 +5480,8 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
if (virDomainDefPostParseMemory(def, data->parseFlags) < 0)
return -1;
+ virDomainDefPostParseOs(def);
+
virDomainDefPostParseMemtune(def);
if (virDomainDefRejectDuplicateControllers(def) < 0)
@@ -18706,7 +18723,7 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
if (type_str) {
int type;
- if ((type = virDomainLoaderTypeFromString(type_str)) < 0) {
+ if ((type = virDomainLoaderTypeFromString(type_str)) <= 0) {
virReportError(VIR_ERR_XML_DETAIL,
_("unknown type value: %s"), type_str);
goto cleanup;
@@ -27611,12 +27628,14 @@ virDomainLoaderDefFormat(virBufferPtr buf,
if (loader->secure)
virBufferAsprintf(buf, " secure='%s'", secure);
- virBufferAsprintf(buf, " type='%s'", type);
+ if (loader->type != VIR_DOMAIN_LOADER_TYPE_NONE)
+ virBufferAsprintf(buf, " type='%s'", type);
if (loader->path)
virBufferEscapeString(buf, ">%s</loader>\n", loader->path);
else
virBufferAddLit(buf, "/>\n");
+
if (loader->nvram || loader->templt) {
virBufferAddLit(buf, "<nvram");
virBufferEscapeString(buf, " template='%s'", loader->templt);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 1f8454b38c..4e8c02b5e3 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1899,7 +1899,8 @@ struct _virDomainBIOSDef {
};
typedef enum {
- VIR_DOMAIN_LOADER_TYPE_ROM = 0,
+ VIR_DOMAIN_LOADER_TYPE_NONE = 0,
+ VIR_DOMAIN_LOADER_TYPE_ROM,
VIR_DOMAIN_LOADER_TYPE_PFLASH,
VIR_DOMAIN_LOADER_TYPE_LAST
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 74f34af292..92729485ff 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9846,6 +9846,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
}
break;
+ case VIR_DOMAIN_LOADER_TYPE_NONE:
case VIR_DOMAIN_LOADER_TYPE_LAST:
/* nada */
break;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index cf7e650b34..cc3a01397c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -12201,6 +12201,7 @@ qemuDomainSetupLoader(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
goto cleanup;
break;
+ case VIR_DOMAIN_LOADER_TYPE_NONE:
case VIR_DOMAIN_LOADER_TYPE_LAST:
break;
}
diff --git a/tests/domaincapsschemadata/full.xml b/tests/domaincapsschemadata/full.xml
index eafba1ae5b..0a46e6bb78 100644
--- a/tests/domaincapsschemadata/full.xml
+++ b/tests/domaincapsschemadata/full.xml
@@ -10,6 +10,7 @@
<value>/foo/bar</value>
<value>/tmp/my_path</value>
<enum name='type'>
+ <value>none</value>
<value>rom</value>
<value>pflash</value>
</enum>
--
2.19.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 02/27/19 11:04, Michal Privoznik wrote:
> This is going to extend virDomainLoader enum. The reason is that
> once loader path is NULL its type makes no sense. However, since
> value of zero corresponds to VIR_DOMAIN_LOADER_TYPE_ROM the
> following XML would be produced:
>
> <os>
> <loader type='rom'/>
> ...
> </os>
>
> To solve this, introduce VIR_DOMAIN_LOADER_TYPE_NONE which would
> correspond to value of zero and then use post parse callback to
> set the default loader type to 'rom' if needed.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/conf/domain_conf.c | 23 +++++++++++++++++++++--
> src/conf/domain_conf.h | 3 ++-
> src/qemu/qemu_command.c | 1 +
> src/qemu/qemu_domain.c | 1 +
> tests/domaincapsschemadata/full.xml | 1 +
> 5 files changed, 26 insertions(+), 3 deletions(-)
Sounds pretty complex, but I guess it makes sense. If both @type and the
pathname content are missing, then @type will default to NONE. (This
used not to be possible, given that pathname used to be required.) If
@type is absent but the pathname is present, then we flip the type
manually to ROM.
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f622a4dddf..b436b91c66 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1013,6 +1013,7 @@ VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST,
>
> VIR_ENUM_IMPL(virDomainLoader,
> VIR_DOMAIN_LOADER_TYPE_LAST,
> + "none",
> "rom",
> "pflash",
> );
> @@ -4286,6 +4287,20 @@ virDomainDefPostParseMemory(virDomainDefPtr def,
> }
>
>
> +static void
> +virDomainDefPostParseOs(virDomainDefPtr def)
> +{
> + if (!def->os.loader)
> + return;
> +
> + if (def->os.loader->path &&
> + def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_NONE) {
> + /* By default, loader is type of 'rom' */
> + def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
> + }
> +}
> +
> +
> static void
> virDomainDefPostParseMemtune(virDomainDefPtr def)
> {
> @@ -5465,6 +5480,8 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
> if (virDomainDefPostParseMemory(def, data->parseFlags) < 0)
> return -1;
>
> + virDomainDefPostParseOs(def);
> +
> virDomainDefPostParseMemtune(def);
>
> if (virDomainDefRejectDuplicateControllers(def) < 0)
> @@ -18706,7 +18723,7 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
>
> if (type_str) {
> int type;
> - if ((type = virDomainLoaderTypeFromString(type_str)) < 0) {
> + if ((type = virDomainLoaderTypeFromString(type_str)) <= 0) {
Why is this change necessary? Hm... I assume, due to the enum
auto-generation in libvirt, the introduction of "none" will
automatically cause virDomainLoaderTypeFromString() to recognize "none"
as value 0. But we want to act like "none" isn't a valid value (it's
internal use only). Hence the same error message as before.
I reckon this is OK.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> virReportError(VIR_ERR_XML_DETAIL,
> _("unknown type value: %s"), type_str);
> goto cleanup;
> @@ -27611,12 +27628,14 @@ virDomainLoaderDefFormat(virBufferPtr buf,
> if (loader->secure)
> virBufferAsprintf(buf, " secure='%s'", secure);
>
> - virBufferAsprintf(buf, " type='%s'", type);
> + if (loader->type != VIR_DOMAIN_LOADER_TYPE_NONE)
> + virBufferAsprintf(buf, " type='%s'", type);
>
> if (loader->path)
> virBufferEscapeString(buf, ">%s</loader>\n", loader->path);
> else
> virBufferAddLit(buf, "/>\n");
> +
> if (loader->nvram || loader->templt) {
> virBufferAddLit(buf, "<nvram");
> virBufferEscapeString(buf, " template='%s'", loader->templt);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 1f8454b38c..4e8c02b5e3 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1899,7 +1899,8 @@ struct _virDomainBIOSDef {
> };
>
> typedef enum {
> - VIR_DOMAIN_LOADER_TYPE_ROM = 0,
> + VIR_DOMAIN_LOADER_TYPE_NONE = 0,
> + VIR_DOMAIN_LOADER_TYPE_ROM,
> VIR_DOMAIN_LOADER_TYPE_PFLASH,
>
> VIR_DOMAIN_LOADER_TYPE_LAST
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 74f34af292..92729485ff 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9846,6 +9846,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
> }
> break;
>
> + case VIR_DOMAIN_LOADER_TYPE_NONE:
> case VIR_DOMAIN_LOADER_TYPE_LAST:
> /* nada */
> break;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index cf7e650b34..cc3a01397c 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -12201,6 +12201,7 @@ qemuDomainSetupLoader(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
> goto cleanup;
> break;
>
> + case VIR_DOMAIN_LOADER_TYPE_NONE:
> case VIR_DOMAIN_LOADER_TYPE_LAST:
> break;
> }
> diff --git a/tests/domaincapsschemadata/full.xml b/tests/domaincapsschemadata/full.xml
> index eafba1ae5b..0a46e6bb78 100644
> --- a/tests/domaincapsschemadata/full.xml
> +++ b/tests/domaincapsschemadata/full.xml
> @@ -10,6 +10,7 @@
> <value>/foo/bar</value>
> <value>/tmp/my_path</value>
> <enum name='type'>
> + <value>none</value>
> <value>rom</value>
> <value>pflash</value>
> </enum>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 2/28/19 10:27 AM, Laszlo Ersek wrote:
> On 02/27/19 11:04, Michal Privoznik wrote:
>> This is going to extend virDomainLoader enum. The reason is that
>> once loader path is NULL its type makes no sense. However, since
>> value of zero corresponds to VIR_DOMAIN_LOADER_TYPE_ROM the
>> following XML would be produced:
>>
>> <os>
>> <loader type='rom'/>
>> ...
>> </os>
>>
>> To solve this, introduce VIR_DOMAIN_LOADER_TYPE_NONE which would
>> correspond to value of zero and then use post parse callback to
>> set the default loader type to 'rom' if needed.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> src/conf/domain_conf.c | 23 +++++++++++++++++++++--
>> src/conf/domain_conf.h | 3 ++-
>> src/qemu/qemu_command.c | 1 +
>> src/qemu/qemu_domain.c | 1 +
>> tests/domaincapsschemadata/full.xml | 1 +
>> 5 files changed, 26 insertions(+), 3 deletions(-)
>
> Sounds pretty complex, but I guess it makes sense. If both @type and the
> pathname content are missing, then @type will default to NONE. (This
> used not to be possible, given that pathname used to be required.) If
> @type is absent but the pathname is present, then we flip the type
> manually to ROM.
Right. The whole idea is that after all these patches the following
domain XML should be valid:
<os firmware='efi'>
<loader secure='yes'/>
</os>
This is for inactive domain. While starting the domain libvirt fills in
path and its type. But for inactive XML tehre is no path. Therefore,
there should be no loader type associated with it.
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index f622a4dddf..b436b91c66 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -1013,6 +1013,7 @@ VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST,
>>
>> VIR_ENUM_IMPL(virDomainLoader,
>> VIR_DOMAIN_LOADER_TYPE_LAST,
>> + "none",
>> "rom",
>> "pflash",
>> );
>> @@ -4286,6 +4287,20 @@ virDomainDefPostParseMemory(virDomainDefPtr def,
>> }
>>
>>
>> +static void
>> +virDomainDefPostParseOs(virDomainDefPtr def)
>> +{
>> + if (!def->os.loader)
>> + return;
>> +
>> + if (def->os.loader->path &&
>> + def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_NONE) {
>> + /* By default, loader is type of 'rom' */
>> + def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
>> + }
>> +}
>> +
>> +
>> static void
>> virDomainDefPostParseMemtune(virDomainDefPtr def)
>> {
>> @@ -5465,6 +5480,8 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
>> if (virDomainDefPostParseMemory(def, data->parseFlags) < 0)
>> return -1;
>>
>> + virDomainDefPostParseOs(def);
>> +
>> virDomainDefPostParseMemtune(def);
>>
>> if (virDomainDefRejectDuplicateControllers(def) < 0)
>> @@ -18706,7 +18723,7 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
>>
>> if (type_str) {
>> int type;
>> - if ((type = virDomainLoaderTypeFromString(type_str)) < 0) {
>> + if ((type = virDomainLoaderTypeFromString(type_str)) <= 0) {
>
> Why is this change necessary? Hm... I assume, due to the enum
> auto-generation in libvirt, the introduction of "none" will
> automatically cause virDomainLoaderTypeFromString() to recognize "none"
> as value 0. But we want to act like "none" isn't a valid value (it's
> internal use only). Hence the same error message as before.
Exactly. virDomainLoaderTypeFromString() will take whatever string user
provided and return matching enum value. For instance, for "rom" it
returns VIR_DOMAIN_LOADER_TYPE_ROM, "pflash" is then mapped on
VIR_DOMAIN_LOADER_TYPE_PFLASH. And with this patch "none" would be
mapped to VIR_DOMAIN_LOADER_TYPE_NONE which has the value of 0. And we
don't want to accept "none" in the domain XML, do we? Hence the change.
>
> I reckon this is OK.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks,
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.