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, §ionNodes);
> > + 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
