On Wed, Aug 15, 2018 at 06:06:26PM +0200, Peter Krempa wrote:
> On Wed, Aug 15, 2018 at 17:02:07 +0200, Erik Skultety wrote:
> > So the procedure to detect SEV support works like this:
> > 1) we detect that sev-guest is among the QOM types and set the cap flag
> > 2) we probe the monitor for SEV support
> > - this is tricky, because QEMU with compiled SEV support will always
> > report -object sev-guest and query-sev-capabilities command, that
> > however doesn't mean SEV is supported
> > 3) depending on what the monitor returned, we either keep or clear the
> > capability flag for SEV
> >
> > Commit a349c6c21c6 added an explicit check for "GenericError" in the
> > monitor reply to prevent libvirtd to spam logs about missing
> > 'query-sev-capabilities' command. At the same time though, it returned
> > success in this case which means that we didn't clear the capability
> > flag afterwards and happily formatted SEV into qemuCaps.
> >
> > Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx>
> > ---
> > src/qemu/qemu_monitor_json.c | 9 +++++----
> > tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 -
> > 2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index 3f99f39120..b0963ed887 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -6459,11 +6459,12 @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon,
> > goto cleanup;
> >
> > /* Both -object sev-guest and query-sev-capabilities can be present
> > - * even if SEV is not available */
> > - if (qemuMonitorJSONHasError(reply, "GenericError")) {
> > - ret = 0;
> > + * even if SEV is not available. We have to check for "GenericError" first,
> > + * in order not to spam libvirtd logs.
> > + * NOTE: We return failure here too so that the capability gets cleared
> > + * later */
>
> This should be noted in a comment for the function as for this specific
> case this function will not report an error but will for any other case.
>
> It's also partially weird since the function also reports other errors
> besides allocation errors which any sane caller has to ignore.
>
> ACK if you add a comment block for this function describing this
> weirdness.
>
> You can also come up with a saner detection method e.g. to return a
> boolean via arguments or 1 via return value which says whether it's
> supported or not and reserve -1 for real errors.
^This sounds more plausible to me, so I'll respin with that, along with the
commentary describing the "weirdness".
Thanks,
Erik
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
