RE: [PATCH v13 3/6] Convert QMP capabilities to domain capabilities

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Michal Peter,

Thank you for your comments.

Way 1:
virBufferAsprintf(buf, "<flc>%s</flc>\n", sgx->flc ? "yes" : "no");

Way 2:
    if (sgx->flc) {
        virBufferAsprintf(buf, "<flc>yes</flc>\n");
    } else {
        virBufferAsprintf(buf, "<flc>yes</flc>\n");
    }

For this section, both ways of writing work.

Peter Krempa said: "Don't use the ternary operator ('?'), use a full if/else branch instead or pick a better data structure."
You mean to be more concise use the ternary operator ('?').

I feel like it all makes sense.


> -----Original Message-----
> From: Michal Prívozník <mprivozn@xxxxxxxxxx>
> Sent: Wednesday, July 20, 2022 7:27 PM
> To: Yang, Lin A <lin.a.yang@xxxxxxxxx>; libvir-list@xxxxxxxxxx; Huang, Haibin
> <haibin.huang@xxxxxxxxx>; Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>
> Subject: Re: [PATCH v13 3/6] Convert QMP capabilities to domain
> capabilities
> 
> On 7/1/22 21:14, Lin Yang wrote:
> > From: Haibin Huang <haibin.huang@xxxxxxxxx>
> >
> > the QMP capabilities:
> >   {"return":
> >     {
> >       "sgx": true,
> >       "section-size": 1024,
> >       "flc": true
> >     }
> >   }
> >
> > the domain capabilities:
> >   <sgx>
> >     <flc>yes</flc>
> >     <epc_size>1</epc_size>
> >   </sgx>
> >
> > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> > Signed-off-by: Haibin Huang <haibin.huang@xxxxxxxxx>
> > ---
> >  src/qemu/qemu_capabilities.c                  | 230 ++++++++++++++++++
> >  src/qemu/qemu_capabilities.h                  |   4 +
> >  .../caps_6.2.0.x86_64.replies                 |  30 ++-
> >  .../caps_6.2.0.x86_64.xml                     |   7 +
> >  .../caps_7.0.0.x86_64.replies                 |  34 ++-
> >  .../caps_7.0.0.x86_64.xml                     |  11 +
> >  .../caps_7.1.0.x86_64.replies                 |  34 ++-
> >  .../caps_7.1.0.x86_64.xml                     |  11 +
> >  8 files changed, 346 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/qemu/qemu_capabilities.c
> > b/src/qemu/qemu_capabilities.c index 2c3be3ecec..57b5acb150 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -671,6 +671,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
> >                "chardev.qemu-vdagent", /*
> QEMU_CAPS_CHARDEV_QEMU_VDAGENT */
> >                "display-dbus", /* QEMU_CAPS_DISPLAY_DBUS */
> >                "iothread.thread-pool-max", /*
> > QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX */
> > +              "sgx-epc", /* QEMU_CAPS_SGX_EPC */
> >      );
> >
> >
> > @@ -752,6 +753,8 @@ struct _virQEMUCaps {
> >
> >      virSEVCapability *sevCapabilities;
> >
> > +    virSGXCapability *sgxCapabilities;
> > +
> >      /* Capabilities which may differ depending on the accelerator. */
> >      virQEMUCapsAccel kvm;
> >      virQEMUCapsAccel hvf;
> > @@ -1394,6 +1397,7 @@ struct virQEMUCapsStringFlags
> virQEMUCapsObjectTypes[] = {
> >      { "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
> >      { "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
> >      { "virtio-iommu-pci", QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI },
> > +    { "sgx-epc", QEMU_CAPS_SGX_EPC },
> >  };
> >
> >
> > @@ -1972,6 +1976,32 @@ virQEMUCapsSEVInfoCopy(virSEVCapability
> **dst,
> > }
> >
> >
> > +static int
> > +virQEMUCapsSGXInfoCopy(virSGXCapability **dst,
> > +                       virSGXCapability *src) {
> > +    g_autoptr(virSGXCapability) tmp = NULL;
> > +
> 
> For a convenience of caller, we can have a src == NULL check here and return
> early.
> 
> > +    tmp = g_new0(virSGXCapability, 1);
> > +
> > +    tmp->flc = src->flc;
> > +    tmp->sgx1 = src->sgx1;
> > +    tmp->sgx2 = src->sgx2;
> > +    tmp->section_size = src->section_size;
> > +
> > +    if (src->nSections == 0) {
> > +        tmp->nSections = 0;
> > +        tmp->pSections = NULL;
> > +    } else {
> > +        tmp->nSections = src->nSections;
> > +        tmp->pSections = src->pSections;
> 
> Ouch, this will definitely lead to a double free. If I run qemucapabilitiestest
> after this patch I can see it clearly. I don't quite understand why you didn't
> though. Fortunately, we can use plain
> memcpy() since virSection struct does not contain any pointer, just a pair of
> integers.
> 
> > +    }
> > +
> > +    *dst = g_steal_pointer(&tmp);
> > +    return 0;
> > +}
> > +
> > +
> >  static void
> >  virQEMUCapsAccelCopyMachineTypes(virQEMUCapsAccel *dst,
> >                                   virQEMUCapsAccel *src) @@ -2053,6
> > +2083,12 @@ virQEMUCaps *virQEMUCapsNewCopy(virQEMUCaps
> *qemuCaps)
> >                                 qemuCaps->sevCapabilities) < 0)
> >          return NULL;
> >
> > +
> > +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC) &&
> > +        virQEMUCapsSGXInfoCopy(&ret->sgxCapabilities,
> > +                               qemuCaps->sgxCapabilities) < 0)
> > +        return NULL;
> > +
> >      return g_steal_pointer(&ret);
> >  }
> >
> > @@ -2091,6 +2127,7 @@ void virQEMUCapsDispose(void *obj)
> >      virCPUDataFree(qemuCaps->cpuData);
> >
> >      virSEVCapabilitiesFree(qemuCaps->sevCapabilities);
> > +    virSGXCapabilitiesFree(qemuCaps->sgxCapabilities);
> >
> >      virQEMUCapsAccelClear(&qemuCaps->kvm);
> >      virQEMUCapsAccelClear(&qemuCaps->hvf);
> > @@ -2616,6 +2653,13 @@ virQEMUCapsGetSEVCapabilities(virQEMUCaps
> > *qemuCaps)  }
> >
> >
> > +virSGXCapabilityPtr
> > +virQEMUCapsGetSGXCapabilities(virQEMUCaps *qemuCaps) {
> > +    return qemuCaps->sgxCapabilities; }
> > +
> > +
> >  static int
> >  virQEMUCapsProbeQMPCommands(virQEMUCaps *qemuCaps,
> >                              qemuMonitor *mon) @@ -3442,6 +3486,31 @@
> > virQEMUCapsProbeQMPSEVCapabilities(virQEMUCaps *qemuCaps,  }
> >
> >
> > +static int
> > +virQEMUCapsProbeQMPSGXCapabilities(virQEMUCaps *qemuCaps,
> > +                                   qemuMonitor *mon) {
> > +    int rc = -1;
> > +    virSGXCapability *caps = NULL;
> > +
> > +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC))
> > +        return 0;
> > +
> > +    if ((rc = qemuMonitorGetSGXCapabilities(mon, &caps)) < 0)
> > +        return -1;
> > +
> > +    /* SGX isn't actually supported */
> > +    if (rc == 0) {
> > +        virQEMUCapsClear(qemuCaps, QEMU_CAPS_SGX_EPC);
> > +        return 0;
> > +    }
> > +
> > +    virSGXCapabilitiesFree(qemuCaps->sgxCapabilities);
> > +    qemuCaps->sgxCapabilities = caps;
> > +    return 0;
> > +}
> > +
> > +
> >  /*
> >   * Filter for features which should never be passed to QEMU. Either
> because
> >   * QEMU never supported them or they were dropped as they never did
> > anything @@ -4220,6 +4289,116 @@
> virQEMUCapsParseSEVInfo(virQEMUCaps
> > *qemuCaps, xmlXPathContextPtr ctxt)  }
> >
> >
> > +static int
> > +virQEMUCapsParseSGXInfo(virQEMUCaps *qemuCaps,
> > +                        xmlXPathContextPtr ctxt) {
> > +    g_autoptr(virSGXCapability) sgx = NULL;
> > +    xmlNodePtr node;
> > +
> > +    g_autofree xmlNodePtr *nodes = NULL;
> > +    g_autofree xmlNodePtr *sectionNodes = NULL;
> > +    g_autofree char *flc = NULL;
> > +    g_autofree char *sgx1 = NULL;
> > +    g_autofree char *sgx2 = NULL;
> > +
> > +    int n = 0;
> > +    int nsections = 0;
> 
> No need for extra empty lines. It's okay if this is one block.
> 
> > +
> > +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC))
> > +        return 0;
> > +
> > +    if (virXPathBoolean("boolean(./sgx)", ctxt) == 0) {
> > +        virReportError(VIR_ERR_XML_ERROR, "%s",
> > +                       _("missing SGX platform data in QEMU capabilities cache"));
> > +        return -1;
> > +    }
> > +
> > +    sgx = g_new0(virSGXCapability, 1);
> > +
> > +    if ((!(flc = virXPathString("string(./sgx/flc)", ctxt))) ||
> > +        virStringParseYesNo(flc, &sgx->flc) < 0) {
> > +        virReportError(VIR_ERR_XML_ERROR, "%s",
> > +                       _("missing or invalid SGX platform flc in QEMU capabilities
> cache"));
> > +        return -1;
> > +    }
> > +
> > +    if ((!(sgx1 = virXPathString("string(./sgx/sgx1)", ctxt))) ||
> > +        virStringParseYesNo(sgx1, &sgx->sgx1) < 0) {
> > +        virReportError(VIR_ERR_XML_ERROR, "%s",
> > +                       _("missing or invalid SGX platform sgx1 in QEMU capabilities
> cache"));
> > +        return -1;
> > +    }
> > +
> > +    if ((!(sgx2 = virXPathString("string(./sgx/sgx2)", ctxt))) ||
> > +        virStringParseYesNo(sgx2, &sgx->sgx2) < 0) {
> > +        virReportError(VIR_ERR_XML_ERROR, "%s",
> > +                       _("missing or invalid SGX platform sgx2 in QEMU capabilities
> cache"));
> > +        return -1;
> > +    }
> > +
> > +    if (virXPathULongLong("string(./sgx/section_size)", ctxt, &sgx-
> >section_size) < 0) {
> > +        virReportError(VIR_ERR_XML_ERROR, "%s",
> > +                       _("missing or malformed SGX platform section_size in QEMU
> capabilities cache"));
> > +        return -1;
> > +    }
> > +
> > +    if ((n = virXPathNodeSet("./sgx/sections", ctxt, &nodes)) < 0) {
> 
> Here, were intrested in a single occurrance, thus virXPathNode() could be
> used.
> 
> > +        sgx->nSections = 0;
> > +        sgx->pSections = NULL;
> > +        VIR_INFO("Sections was not obtained, so QEMU version is
> > + 6.2.0");
> 
> Again, no need for extra NOPs, VIR_INFO()...
> 
> > +        qemuCaps->sgxCapabilities = g_steal_pointer(&sgx);
> > +        return 0;
> > +    }
> > +
> > +    if (n == 0) {
> > +        qemuCaps->sgxCapabilities = g_steal_pointer(&sgx);
> > +        return 0;
> > +    }
> > +
> > +    // Got the section, the QEMU version is above 7.0.0
> 
> We like c89 style of comments.
> 
> > +    node = ctxt->node;
> > +    ctxt->node = nodes[0];
> > +    nsections = virXPathNodeSet("./section", ctxt, &sectionNodes);
> > +    ctxt->node = node;
> > +
> > +    if (nsections < 0) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("failed to parse CPU blockers in QEMU capabilities"));
> > +        return -1;
> > +    }
> > +
> > +    if (nsections > 0) {
> > +        size_t i;
> > +        g_autofree char * strNode = NULL;
> > +        g_autofree char * strSize = NULL;
> > +        sgx->nSections = nsections;
> > +        sgx->pSections = g_new0(virSection, nsections + 1);
> > +
> > +        for (i = 0; i < nsections; i++) {
> > +            if ((strNode = virXMLPropString(sectionNodes[i], "node")) &&
> > +                (virStrToLong_ui(strNode, NULL, 10, &(sgx->pSections[i].node)) <
> 0)) {
> > +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                                _("missing node name in QEMU "
> > +                                    "capabilities cache"));
> 
> Error messages are extempt from 80 chars line rule. The reason is:
> simpler git-grep. I, as a developer, can take whatever portion of error
> message and 'git grep' it and find it easily. But if the message is broken into
> multiple lines it's more tricky to do.
> 
> > +                return -1;
> > +            }
> > +
> > +            if ((strSize = virXMLPropString(sectionNodes[i], "size")) &&
> > +                (virStrToLong_ull(strSize, NULL, 10, &(sgx->pSections[i].size)) < 0))
> {
> > +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                                _("missing size name in QEMU "
> > +                                    "capabilities cache"));
> > +                return -1;
> > +            }
> > +        }
> > +    }
> > +
> > +    qemuCaps->sgxCapabilities = g_steal_pointer(&sgx);
> > +    return 0;
> > +}
> > +
> > +
> >  static int
> >  virQEMUCapsParseFlags(virQEMUCaps *qemuCaps, xmlXPathContextPtr
> ctxt)
> > { @@ -4522,6 +4701,9 @@ virQEMUCapsLoadCache(virArch hostArch,
> >      if (virQEMUCapsParseSEVInfo(qemuCaps, ctxt) < 0)
> >          return -1;
> >
> > +    if (virQEMUCapsParseSGXInfo(qemuCaps, ctxt) < 0)
> > +        return -1;
> > +
> >      if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
> >          virQEMUCapsInitHostCPUModel(qemuCaps, hostArch,
> VIR_DOMAIN_VIRT_KVM);
> >      if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF)) @@ -4707,6
> +4889,49
> > @@ virQEMUCapsFormatSEVInfo(virQEMUCaps *qemuCaps, virBuffer
> *buf)  }
> >
> >
> > +static void
> > +virQEMUCapsFormatSGXInfo(virQEMUCaps *qemuCaps,
> > +                         virBuffer *buf) {
> > +    virSGXCapabilityPtr sgx = virQEMUCapsGetSGXCapabilities(qemuCaps);
> > +    size_t i;
> > +
> > +    virBufferAddLit(buf, "<sgx supported='yes'>\n");
> > +    virBufferAdjustIndent(buf, 2);
> > +    if (sgx->flc) {
> > +        virBufferAsprintf(buf, "<flc>%s</flc>\n", "yes");
> > +    } else {
> > +        virBufferAsprintf(buf, "<flc>%s</flc>\n", "no");
> > +    }
> 
> Well, this works. But how about:
> 
> if (sgx->flc) {
>   virBufferAddLit(buf, "<flc>yes</flc>\n"); } else {
>   virBufferAddLit(buf, "<flc>no</flc>\n"); }
> 
> which saves and extra call to printf() for a static string? Or even better:
> 
>   virBufferAsprintf(buf, "<flc>%s</flc>\n", sgx->flc ? "yes" : "no");
> 
> > +    if (sgx->sgx1) {
> > +        virBufferAsprintf(buf, "<sgx1>%s</sgx1>\n", "yes");
> > +    } else {
> > +        virBufferAsprintf(buf, "<sgx1>%s</sgx1>\n", "no");
> > +    }
> > +    if (sgx->sgx2) {
> > +        virBufferAsprintf(buf, "<sgx2>%s</sgx2>\n", "yes");
> > +    } else {
> > +        virBufferAsprintf(buf, "<sgx2>%s</sgx2>\n", "no");
> > +    }
> > +    virBufferAsprintf(buf, "<section_size
> > + unit='KiB'>%llu</section_size>\n", sgx->section_size);
> > +
> > +    if (sgx->nSections > 0) {
> > +        virBufferAddLit(buf, "<sections>\n");
> > +
> > +        for (i = 0; i < sgx->nSections; i++) {
> > +            virBufferAdjustIndent(buf, 2);
> > +            virBufferAsprintf(buf, "<section node='%u' ", sgx-
> >pSections[i].node);
> > +            virBufferAsprintf(buf, "size='%llu'/>\n", sgx->pSections[i].size);
> > +            virBufferAdjustIndent(buf, -2);
> > +        }
> > +        virBufferAddLit(buf, "</sections>\n");
> > +    }
> > +
> > +    virBufferAdjustIndent(buf, -2);
> > +    virBufferAddLit(buf, "</sgx>\n"); }
> > +
> > +
> >  char *
> >  virQEMUCapsFormatCache(virQEMUCaps *qemuCaps)  { @@ -4788,6
> +5013,9
> > @@ virQEMUCapsFormatCache(virQEMUCaps *qemuCaps)
> >      if (qemuCaps->sevCapabilities)
> >          virQEMUCapsFormatSEVInfo(qemuCaps, &buf);
> >
> > +    if (qemuCaps->sgxCapabilities)
> > +        virQEMUCapsFormatSGXInfo(qemuCaps, &buf);
> > +
> >      if (qemuCaps->kvmSupportsNesting)
> >          virBufferAddLit(&buf, "<kvmSupportsNesting/>\n");
> >
> > @@ -5455,6 +5683,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCaps
> *qemuCaps,
> >          return -1;
> >      if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0)
> >          return -1;
> > +    if (virQEMUCapsProbeQMPSGXCapabilities(qemuCaps, mon) < 0)
> > +        return -1;
> >
> >      virQEMUCapsInitProcessCaps(qemuCaps);
> >
> > diff --git a/src/qemu/qemu_capabilities.h
> > b/src/qemu/qemu_capabilities.h index 6f35ba1485..fc8c0fde1b 100644
> > --- a/src/qemu/qemu_capabilities.h
> > +++ b/src/qemu/qemu_capabilities.h
> > @@ -650,6 +650,7 @@ typedef enum { /* virQEMUCapsFlags grouping
> marker for syntax-check */
> >      QEMU_CAPS_CHARDEV_QEMU_VDAGENT, /* -chardev qemu-vdagent
> */
> >      QEMU_CAPS_DISPLAY_DBUS, /* -display dbus */
> >      QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX, /* -object
> > iothread.thread-pool-max */
> > +    QEMU_CAPS_SGX_EPC, /* -object sgx-epc,... */
> >
> >      QEMU_CAPS_LAST /* this must always be the last item */  }
> > virQEMUCapsFlags; @@ -843,6 +844,9 @@
> > virQEMUCapsCPUFeatureFromQEMU(virQEMUCaps *qemuCaps,
> virSEVCapability
> > *  virQEMUCapsGetSEVCapabilities(virQEMUCaps *qemuCaps);
> >
> > +virSGXCapabilityPtr
> > +virQEMUCapsGetSGXCapabilities(virQEMUCaps *qemuCaps);
> > +
> >  bool
> >  virQEMUCapsGetKVMSupportsSecureGuest(virQEMUCaps *qemuCaps)
> > G_GNUC_NO_INLINE;
> >
> > diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
> > b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
> > index e235532d62..0151ab07fa 100644
> > --- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
> > +++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
> > @@ -7459,15 +7459,15 @@
> >            "type": "bool"
> >          },
> >          {
> > -          "name": "sgx1",
> > +          "name": "flc",
> >            "type": "bool"
> >          },
> >          {
> > -          "name": "sgx2",
> > +          "name": "sgx1",
> >            "type": "bool"
> >          },
> >          {
> > -          "name": "flc",
> > +          "name": "sgx2",
> >            "type": "bool"
> >          },
> >          {
> 
> This move does not seem to be warranted. When Peter generated the file
> I'm quite certain that this was genuine order of attributes in which QEMU
> replied. Furthermore, nothing in our code relies on ordering of these
> particular attributes. Why is this necessary?
> 
> In fact, the QEMU I've built from git recently (v7.0.0-2668-gf9d9fff72e)
> replied in this order:
> 
> {"return": {"sgx": true, "sgx2": false, "sgx1": true, "sections":
> [{"size": 98041856, "node": 0}], "section-size": 98041856, "flc":
> false}, "id": "libvirt-50"}
> 
> Michal





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux