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 <phrd...@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 :|
