9:57 a.m.
On Mon, Jun 14, 2021 at 16:13:40 +0200, Pavel Hrdina wrote:
On Fri, Jun 11, 2021 at 04:18:24PM +0200, Peter Krempa wrote:
> On Fri, Jun 11, 2021 at 15:24:09 +0200, Pavel Hrdina wrote:
> > In libvirt we already use `query-command-line-options` QMP command but
> > that is useless as it doesn't provide correct data for `-machine`
> > option. So we need a new and better way to get that data.
> >
> > We already use `qom-list-properties` to get options for specific machine
> > types so we can reuse it to get options for special `none` machine type
> > as a generic arch independent machine type.
> >
> > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>
>
> [...]
>
> >
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 2b9ab9af60..436fe40f65 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -632,6 +632,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
> > "input-linux",
> > "virtio-gpu-gl-pci",
> > "virtio-vga-gl",
> > +
> > + /* 405 */
> > + "confidential-guest-support",
> > );
> >
> >
> > @@ -1756,6 +1759,10 @@ static struct virQEMUCapsStringFlags
virQEMUCapsMachinePropsVirt[] = {
> > { "iommu", QEMU_CAPS_MACHINE_VIRT_IOMMU },
> > };
> >
> > +static struct virQEMUCapsStringFlags virQEMUCapsMachinePropsGeneric[] = {
> > + { "confidential-guest-support",
QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT },
>
> If possible please include this line and all the detection that happens
> from it in a separate commit including the definition of the new
> capability flag.
>
> > +};
> > +
> > static virQEMUCapsObjectTypeProps virQEMUCapsMachineProps[] = {
> > { "pseries", virQEMUCapsMachinePropsPSeries,
> > G_N_ELEMENTS(virQEMUCapsMachinePropsPSeries),
> > @@ -1763,6 +1770,9 @@ static virQEMUCapsObjectTypeProps
virQEMUCapsMachineProps[] = {
> > { "virt", virQEMUCapsMachinePropsVirt,
> > G_N_ELEMENTS(virQEMUCapsMachinePropsVirt),
> > -1 },
> > + { "none", virQEMUCapsMachinePropsGeneric,
> > + G_N_ELEMENTS(virQEMUCapsMachinePropsGeneric),
> > + -1 },
> > };
> >
> > static void
> > @@ -2865,8 +2875,10 @@ virQEMUCapsProbeQMPMachineProps(virQEMUCaps *qemuCaps,
> > const char *canon = virQEMUCapsGetCanonicalMachine(qemuCaps, virtType,
props.type);
> > g_autofree char *type = NULL;
> >
> > - if (!virQEMUCapsIsMachineSupported(qemuCaps, virtType, canon))
> > + if (STRNEQ(canon, "none") &&
> > + !virQEMUCapsIsMachineSupported(qemuCaps, virtType, canon)) {
> > continue;
>
> This looks fishy since after this patch we'll ever only skip the 'none'
> machine, whereas previously we'd let get virQEMUCapsIsMachineSupported
> called.
No true, see the following matrix:
A (canon != "none")
B (!virQEMUCapsIsMachineSupported())
A B A & B
0 0 0
0 1 0
1 0 0
1 1 1
The current code will skip the call only if `canon` is not `none` and
`canon` is not supported.
In all other cases we want to call to QEMU and get the machine type
props.
Essentially we want to do the call only if the machine type is "none"
or it is supported and skip it in all other cases which can be written
like this:
if (!(STREQ(canon, "none") || virQEMUCapsIsMachineSupported()))
and with a bit of boolean algebra you will get
if (!STREQ(canon, "none") && !virQEMUCapsIsMachineSupported())
where for !STREQ() we have STRNEQ().
So unless I'm missing something the condition is correct.
Hmm, yeah. I've probably originally read it as
STREQ(..."none") && !virQEMUCapsIsMachineSupported
(STREQ vs STRNEQ)
In such case we'd only ever check and skip "none" machines which would
be wrong, but yours is correct.
Pavel
> > + }
> >
> > /* The QOM type for machine types is the machine type name
> > * followed by the -machine suffix */
> > diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies
b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies
> > index 884db53152..aa8209cc42 100644
> > --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies
> > +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies
> > @@ -17523,9 +17523,122 @@
> > "id": "libvirt-36"
> > }
> >
> > +{
> > + "execute": "qom-list-properties",
> > + "arguments": {
> > + "typename": "none-machine"
> > + },
> > + "id": "libvirt-37"
> > +}
>
> As noted above, I'd prefer if the update of replies is separated from
> addition of the capability.
>