[libvirt][PATCH v11 1/4] qemu: provide support to query the SGX capability

Daniel P. Berrangé berrange at redhat.com
Mon Jul 25 12:37:48 UTC 2022


On Mon, Jul 25, 2022 at 02:34:33PM +0200, Michal Prívozník wrote:
> On 5/11/22 18:05, Peter Krempa wrote:
> > On Tue, May 10, 2022 at 23:11:09 -0700, Lin Yang wrote:
> >> From: Haibin Huang <haibin.huang at intel.com>
> >>
> >> QEMU version >= 6.2.0 provides support for creating enclave on
> >> SGX x86 platform using Software Guard Extensions (SGX) feature.
> >> This patch adds support to query the SGX capability from the qemu.
> >>
> >> Signed-off-by: Haibin Huang <haibin.huang at intel.com>
> >> ---
> >>  src/conf/domain_capabilities.c                |  10 ++
> >>  src/conf/domain_capabilities.h                |  13 ++
> >>  src/libvirt_private.syms                      |   1 +
> >>  src/qemu/qemu_capabilities.c                  | 119 ++++++++++++++++++
> >>  src/qemu/qemu_capabilities.h                  |   6 +
> >>  src/qemu/qemu_capspriv.h                      |   4 +
> >>  src/qemu/qemu_monitor.c                       |  10 ++
> >>  src/qemu/qemu_monitor.h                       |   3 +
> >>  src/qemu/qemu_monitor_json.c                  | 104 +++++++++++++--
> >>  src/qemu/qemu_monitor_json.h                  |   9 ++
> >>  .../caps_6.2.0.x86_64.replies                 |  22 +++-
> >>  .../caps_6.2.0.x86_64.xml                     |   5 +
> >>  .../caps_7.0.0.x86_64.replies                 |  22 +++-
> >>  .../caps_7.0.0.x86_64.xml                     |   5 +
> >>  14 files changed, 318 insertions(+), 15 deletions(-)
> > 
> > This is not a full review. Couple of points:
> > 
> > 1) Do not mix other changes with adding QEMU_CAPS* stuff
> >     Basically theres waaay too much going on in this patch and it
> >     definitely can be separated into smaller chunks. The QEMU_CAPS is
> >     just one of them.
> >     Separate at least:
> >         - qemu monitor command introduction
> >         - domain capabilities data structs for sgx
> >         - parsing and formatting of the XML
> >         - adding of the QEMU_CAPS_ flag
> > 2) caps for qemu-7.1 were added very recently
> >     You'll need to fix that one too since you added an extra query. Make
> >     sure that you _don't_ add the faking of SXG into that file, but
> >     rather the error case. My box doesn't support SGX so it will be
> >     overwritten in my next refresh anyways.
> > 
> > [...]
> > 
> >> @@ -4706,6 +4805,21 @@ virQEMUCapsFormatSEVInfo(virQEMUCaps *qemuCaps, virBuffer *buf)
> >>  }
> >>  
> >>  
> >> +static void
> >> +virQEMUCapsFormatSGXInfo(virQEMUCaps *qemuCaps,
> >> +                         virBuffer *buf)
> >> +{
> >> +    virSGXCapabilityPtr sgx = virQEMUCapsGetSGXCapabilities(qemuCaps);
> >> +
> >> +    virBufferAddLit(buf, "<sgx>\n");
> >> +    virBufferAdjustIndent(buf, 2);
> >> +    virBufferAsprintf(buf, "<flc>%s</flc>\n", sgx->flc ? "yes" : "no");
> > 
> > Don't use the ternary operator ('?'), use a full if/else branch instead
> > or pick a better data structure.
> 
> I wish our coding style was more forgiving about ternary operator.
> Because in this specific case we have bad and worse options:
> 
> 1) Expand to if-else fully:
> 
>   if (boolVar)
>     virBufferAddLit(buf, "<elem>yes</elem>\n");
>   else
>     virBufferAddLit(buf, "<elem>no</elem>\n");
> 
> 2) use virTristateBoolFromBool()
> 
>   virBufferAsprintf(buf, "<elem>%s</elem>\n",
> virTristateBoolTypeToString(virTristateBoolFromBool(boolVar)));
> 
> 3) use a temporary variable:
> 
>    char *tmp;
>    if (boolVar)
>      tmp = "yes";
>    else
>      tmp = "no";
> 
>   virBufferAsprintf(buf, "<elem>%s</elem>\n", tmp);
> 
> 4) use virTristateBool:
> 
>   if (virTristateVar)
>     virBudderAsprintf(buf, "<elem>%s</elem>\n",
> virTristateBoolTypeToSring(virTristateVar));
> 
> 5) Introduce a helper, e.g.
> 
> const char *
> virYesNoString(bool val) {
>   if (val)
>     return "yes";
>   return "no";
> }
> 
>   virBufferAsprintf(buf, "<elem>%s</elem>\n", virYesNoString(boolVar));
> 
> Frankly speaking, I dislike 2) the most, followed by 1, 3, 4, 5.
> According to my understanding, we discourage use of ternary operator
> because it can lead to hard to parse expressions. But one can hardly
> argue about bool ? "yes" : "no" being hard to parse.

We use the ternary operator all over the code for this exact pattern
and it is just fine. As you say all these other options above are
worse than what this patch has.  The style guide needs changing to
make this usage clearly acceptable.


With 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 :|


More information about the libvir-list mailing list