[libvirt PATCH 8/9] conf: introduce support for firmware auto-selection feature filtering

[Kashyap Chamarthy]

On Fri, Mar 19, 2021 at 11:59:11AM +0100, Pavel Hrdina wrote:
> On Fri, Mar 19, 2021 at 11:10:05AM +0100, Kashyap Chamarthy wrote:
> > On Thu, Mar 18, 2021 at 01:26:45PM +0100, Pavel Hrdina wrote:

[...]

> > Nit: I'd recast it as: "When using firmware auto-selection, different
> > features are enabled in any given firmware binary."
> 
> Sounds a bit better but I've already pushed the patches.

Np; can be a follow-up.

[...]

> > Should we also list a couple of example features?  E.g.  "amd-sev" (on
> > supported hardware), "acpi-s3", "secure-boot".
> 
> I was considering listing all features that the JSON files can have but
> most of the other features are already controlled by different XML
> elements. There is an explicit list of features later in the docs.

Ah, where's the explict list of features?  I don't see them under the
"BIOS bootloader" section:
https://libvirt.org/formatdomain.html#bios-bootloader

> > I notice that "enrolled-keys" is the only feature advertized by one of
> > the firmware descriptor files (40-edk2-ovmf-x64-sb-enrolled.json), which
> > is used as a mandatory XML attribute for the ``feature`` element.
> > 
> > I'm silently wondering there's a possibility for confusion: "Hmm,
> > 'enrolled-keys' is a possible feature, but it is a mandatory attribute
> > of ``feature`` element."
> 
> I don't understand what you mean here. If you are talking about our XML
> and the following text here in this patch then if you use the feature
> element both attributes ``enabled`` and ``name`` are mandatory. 
> But few lines above there is a statement that the you can specify zero
> or more ``feature`` elements so you don't have to limit any features.

I missed the "zero or more" bit; sorry.

> Our documentation doesn't state that 'enrolled-keys' is possible feature
> and mandatory attribute at the same time.

Yep, fair enough.  You can disregard my second paragraph above.

[...]

> > > +        - ``enrolled-keys`` whether the selected nvram template has default
> > 
> > Nit: s/nvram/NVRAM/  (Or ``nvram``, to use rST verbatim; as it's an XML
> > element.)
> 
> Using ``nvram`` would have been probably better.

[...]

> >     Acked-by: Kashyap Chamarthy <kchamart at redhat.com>   
> 
> As I stated the patch is already pushed so you can post a patch fixing
> the small nits listed here.

Sure.

> Thanks for the review.
> 
> Pavel



-- 
/kashyap