[libvirt] [PATCH 3/4] qemu: Fix probing of AMD SEV support

[Peter Krempa]

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 at redhat.com>
> ---
>  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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180815/a2b23c02/attachment-0001.sig>