Re: [libvirt PATCH] conf: remove duplicated firmware type attribute
On Mon, Mar 29, 2021 at 08:20:32PM +0200, Pavel Hrdina wrote:
On Mon, Mar 29, 2021 at 07:03:52PM +0100, Daniel P. Berrangé wrote:
> The
>
> <os firmware='efi'>
> <firmware type='efi'>
> <feature enabled='no' name='enrolled-keys'/>
> </firmware>
> </os>
>
> repeats the firmware attribute twice. This has no functional benefit, as
> evidenced by fact that we use a single struct field to store both
> attributes, while needlessly introducing an error scenario. The XML can
> just be simplified to:
>
> <os firmware='efi'>
> <firmware>
> <feature enabled='no' name='enrolled-keys'/>
> </firmware>
> </os>
>
> which also means that we don't need to emit the empty element
> <firmware type='efi'/> for all existing configs too.
My original motivation was that if we ever need to introduce another
attribute to the <firmware> element it would be nicely grouped together.
But I guess it wound not be a big deal if we would have:
<os firmware='efi'>
<firmware someAttr='value'>
<feature enabled='no' name='enrolled-keys'/>
</firmware>
</os>
This would look reasonable as well so
This is pretty much the situation we're already in for a number of existing
elements too. For example <disk> has type=file|block|network on the top
level, but then the variable content is under the child <source> element.
If we didnt already have @firmware on the <os> element it would make sense
to have @type on <firmware>, but given existing schema, avoiding duplicating
information is better.
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|