Re: [PATCH 2/7] conf: support crypto device

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

 



On 1/4/23 04:29, zhenwei pi wrote:
> Support a new device type 'crypto'.
> 
> Signed-off-by: zhenwei pi <pizhenwei@xxxxxxxxxxxxx>
> ---
>  src/conf/domain_conf.c         | 191 +++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h         |  40 +++++++
>  src/conf/domain_postparse.c    |   1 +
>  src/conf/domain_validate.c     |  18 ++++
>  src/conf/virconftypes.h        |   2 +
>  src/libvirt_private.syms       |   1 +
>  src/qemu/qemu_command.c        |   1 +
>  src/qemu/qemu_domain.c         |   3 +
>  src/qemu/qemu_domain_address.c |  26 +++++
>  src/qemu/qemu_driver.c         |   5 +
>  src/qemu/qemu_hotplug.c        |   3 +
>  src/qemu/qemu_validate.c       |  22 ++++
>  12 files changed, 313 insertions(+)

What I'm missing here is qemuxml2xmltest test case. We surely want to
test whether parsing and formatting of the new XML works.

> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6c088ff295..74448fe627 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -332,6 +332,7 @@ VIR_ENUM_IMPL(virDomainDevice,
>                "iommu",
>                "vsock",
>                "audio",
> +              "crypto",
>  );
>  
>  VIR_ENUM_IMPL(virDomainDiskDevice,
> @@ -1314,6 +1315,22 @@ VIR_ENUM_IMPL(virDomainVsockModel,
>                "virtio-non-transitional",
>  );
>  
> +VIR_ENUM_IMPL(virDomainCryptoModel,
> +              VIR_DOMAIN_CRYPTO_MODEL_LAST,
> +              "virtio",
> +);
> +
> +VIR_ENUM_IMPL(virDomainCryptoType,
> +              VIR_DOMAIN_CRYPTO_TYPE_LAST,
> +              "qemu",
> +);
> +
> +VIR_ENUM_IMPL(virDomainCryptoBackend,
> +              VIR_DOMAIN_CRYPTO_BACKEND_LAST,
> +              "builtin",
> +              "lkcf",
> +);
> +
>  VIR_ENUM_IMPL(virDomainDiskDiscard,
>                VIR_DOMAIN_DISK_DISCARD_LAST,
>                "default",
> @@ -3464,6 +3481,9 @@ void virDomainDeviceDefFree(virDomainDeviceDef *def)
>      case VIR_DOMAIN_DEVICE_AUDIO:
>          virDomainAudioDefFree(def->data.audio);
>          break;
> +    case VIR_DOMAIN_DEVICE_CRYPTO:
> +        virDomainCryptoDefFree(def->data.crypto);
> +        break;
>      case VIR_DOMAIN_DEVICE_LAST:
>      case VIR_DOMAIN_DEVICE_NONE:
>          break;
> @@ -3807,6 +3827,10 @@ void virDomainDefFree(virDomainDef *def)
>          virDomainPanicDefFree(def->panics[i]);
>      g_free(def->panics);
>  
> +    for (i = 0; i < def->ncryptos; i++)
> +        virDomainCryptoDefFree(def->cryptos[i]);
> +    g_free(def->cryptos);
> +
>      virDomainIOMMUDefFree(def->iommu);
>  
>      g_free(def->idmap.uidmap);
> @@ -4360,6 +4384,8 @@ virDomainDeviceGetInfo(const virDomainDeviceDef *device)
>          return &device->data.iommu->info;
>      case VIR_DOMAIN_DEVICE_VSOCK:
>          return &device->data.vsock->info;
> +    case VIR_DOMAIN_DEVICE_CRYPTO:
> +        return &device->data.crypto->info;
>  
>      /* The following devices do not contain virDomainDeviceInfo */
>      case VIR_DOMAIN_DEVICE_LEASE:
> @@ -4462,6 +4488,9 @@ virDomainDeviceSetData(virDomainDeviceDef *device,
>      case VIR_DOMAIN_DEVICE_AUDIO:
>          device->data.audio = devicedata;
>          break;
> +    case VIR_DOMAIN_DEVICE_CRYPTO:
> +        device->data.crypto = devicedata;
> +        break;
>      case VIR_DOMAIN_DEVICE_NONE:
>      case VIR_DOMAIN_DEVICE_LAST:
>          break;
> @@ -4673,6 +4702,13 @@ virDomainDeviceInfoIterateFlags(virDomainDef *def,
>              return rc;
>      }
>  
> +    device.type = VIR_DOMAIN_DEVICE_CRYPTO;
> +    for (i = 0; i < def->ncryptos; i++) {
> +        device.data.crypto = def->cryptos[i];
> +        if ((rc = cb(def, &device, &def->cryptos[i]->info, opaque)) != 0)
> +            return rc;
> +    }
> +
>      /* If the flag below is set, make sure @cb can handle @info being NULL */
>      if (iteratorFlags & DOMAIN_DEVICE_ITERATE_MISSING_INFO) {
>          device.type = VIR_DOMAIN_DEVICE_GRAPHICS;
> @@ -4731,6 +4767,7 @@ virDomainDeviceInfoIterateFlags(virDomainDef *def,
>      case VIR_DOMAIN_DEVICE_IOMMU:
>      case VIR_DOMAIN_DEVICE_VSOCK:
>      case VIR_DOMAIN_DEVICE_AUDIO:
> +    case VIR_DOMAIN_DEVICE_CRYPTO:
>          break;
>      }
>  #endif
> @@ -13417,6 +13454,94 @@ virDomainVsockDefParseXML(virDomainXMLOption *xmlopt,
>      return g_steal_pointer(&vsock);
>  }
>  
> +
> +static virDomainCryptoDef *
> +virDomainCryptoDefParseXML(virDomainXMLOption *xmlopt,
> +                           xmlNodePtr node,
> +                           xmlXPathContextPtr ctxt,
> +                           unsigned int flags)
> +{
> +    virDomainCryptoDef *def;
> +    VIR_XPATH_NODE_AUTORESTORE(ctxt)
> +    int nbackends;
> +    g_autofree xmlNodePtr *backends = NULL;
> +    g_autofree char *model = NULL;
> +    g_autofree char *backend = NULL;
> +    g_autofree char *type = NULL;
> +
> +    def = g_new0(virDomainCryptoDef, 1);
> +
> +    if (!(model = virXMLPropString(node, "model"))) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s", _("missing crypto device model"));
> +        goto error;
> +    }
> +
> +    if ((def->model = virDomainCryptoModelTypeFromString(model)) < 0) {

Problem with this is that compiler may decide that def->model is
unsigned (because it's declared as: virDomainCryptoModel model.
Now, if virXXXTypeFromString() fails and returns -1, this is then
typecased into unsigned int (or whatever unsigned type compiler decided
on) and < 0 check is never true.

Fortunately, we have a conencient function for getting attribute values
and translating them into enums: virXMLPropEnum().

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown crypto model '%s'"), model);
> +        goto error;
> +    }
> +
> +    if (!(type = virXMLPropString(node, "type"))) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s", _("missing crypto device type"));
> +        goto error;
> +    }
> +
> +    if ((def->type = virDomainCryptoTypeTypeFromString(type)) < 0) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown crypto type '%s'"), model);
> +        goto error;
> +    }
> +
> +    ctxt->node = node;
> +
> +    if ((nbackends = virXPathNodeSet("./backend", ctxt, &backends)) < 0)
> +        goto error;
> +
> +    if (nbackends != 1) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("only one crypto backend is supported"));
> +        goto error;
> +    }
> +
> +    if (!(backend = virXMLPropString(backends[0], "model"))) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("missing crypto device backend model"));
> +        goto error;
> +    }
> +
> +    if ((def->backend = virDomainCryptoBackendTypeFromString(backend)) < 0) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("unknown crypto backend model '%s'"), backend);
> +        goto error;
> +    }
> +
> +    if (virXMLPropUInt(backends[0], "queues", 10, VIR_XML_PROP_NONE, &def->queues) < 0) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("parsing crypto device queues failed"));

Nope, this overwrites more specific error message reported by
virXMLPropUInt().

> +        goto error;
> +    }
> +
> +    switch ((virDomainCryptoBackend) def->backend) {
> +    case VIR_DOMAIN_CRYPTO_BACKEND_BUILTIN:
> +    case VIR_DOMAIN_CRYPTO_BACKEND_LKCF:
> +    case VIR_DOMAIN_CRYPTO_BACKEND_LAST:
> +        break;
> +    }

What's the purpose of this statement?

> +
> +    if (virDomainDeviceInfoParseXML(xmlopt, node, ctxt, &def->info, flags) < 0)
> +        goto error;
> +
> +    if (virDomainVirtioOptionsParseXML(virXPathNode("./driver", ctxt),
> +                                       &def->virtio) < 0)
> +        goto error;
> +
> +    return def;
> +
> + error:
> +    g_clear_pointer(&def, virDomainCryptoDefFree);

How about declaring @def as g_autoptr() and dropping this label completely?

> +    return NULL;
> +}
> +
> +
>  virDomainDeviceDef *
>  virDomainDeviceDefParse(const char *xmlStr,
>                          const virDomainDef *def,
> @@ -13578,6 +13703,11 @@ virDomainDeviceDefParse(const char *xmlStr,
>                                                            flags)))
>              return NULL;
>          break;
> +    case VIR_DOMAIN_DEVICE_CRYPTO:
> +        if (!(dev->data.crypto = virDomainCryptoDefParseXML(xmlopt, node, ctxt,
> +                                                          flags)))
> +            return NULL;
> +        break;
>      case VIR_DOMAIN_DEVICE_NONE:
>      case VIR_DOMAIN_DEVICE_LAST:
>          break;
> @@ -18670,6 +18800,21 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt,
>      }
>      VIR_FREE(nodes);
>  
> +    /* Parse the crypto devices */
> +    if ((n = virXPathNodeSet("./devices/crypto", ctxt, &nodes)) < 0)
> +        return NULL;
> +    if (n)
> +        def->cryptos = g_new0(virDomainCryptoDef *, n);
> +    for (i = 0; i < n; i++) {
> +        virDomainCryptoDef *crypto = virDomainCryptoDefParseXML(xmlopt, nodes[i],
> +                                                       ctxt, flags);
> +        if (!crypto)
> +            return NULL;
> +
> +        def->cryptos[def->ncryptos++] = crypto;
> +    }
> +    VIR_FREE(nodes);
> +
>      /* Parse the TPM devices */
>      if ((n = virXPathNodeSet("./devices/tpm", ctxt, &nodes)) < 0)
>          return NULL;
> @@ -21210,6 +21355,7 @@ virDomainDefCheckABIStabilityFlags(virDomainDef *src,
>      case VIR_DOMAIN_DEVICE_IOMMU:
>      case VIR_DOMAIN_DEVICE_VSOCK:
>      case VIR_DOMAIN_DEVICE_AUDIO:
> +    case VIR_DOMAIN_DEVICE_CRYPTO:
>          break;
>      }
>  #endif
> @@ -24562,6 +24708,47 @@ virDomainRNGDefFree(virDomainRNGDef *def)
>  }
>  
>  
> +static int
> +virDomainCryptoDefFormat(virBuffer *buf,
> +                         virDomainCryptoDef *def,
> +                         unsigned int flags)
> +{
> +    const char *model = virDomainCryptoModelTypeToString(def->model);
> +    const char *type = virDomainCryptoTypeTypeToString(def->model);
> +    const char *backend = virDomainCryptoBackendTypeToString(def->backend);
> +    g_auto(virBuffer) driverAttrBuf = VIR_BUFFER_INITIALIZER;
> +
> +    virBufferAsprintf(buf, "<crypto model='%s' type='%s'>\n", model, type);
> +    virBufferAdjustIndent(buf, 2);
> +    virBufferAsprintf(buf, "<backend model='%s'", backend);
> +    if (def->queues)
> +        virBufferAsprintf(buf, " queues='%d'", def->queues);
> +    virBufferAddLit(buf, "/>\n");
> +
> +    virDomainVirtioOptionsFormat(&driverAttrBuf, def->virtio);
> +
> +    virXMLFormatElement(buf, "driver", &driverAttrBuf, NULL);
> +
> +    virDomainDeviceInfoFormat(buf, &def->info, flags);
> +
> +    virBufferAdjustIndent(buf, -2);
> +    virBufferAddLit(buf, "</crypto>\n");
> +
> +    return 0;

This is all the function returns. Can this be made void instead?

> +}
> +
> +void
> +virDomainCryptoDefFree(virDomainCryptoDef *def)
> +{
> +    if (!def)
> +        return;
> +
> +    virDomainDeviceInfoClear(&def->info);
> +    g_free(def->virtio);
> +    g_free(def);
> +}
> +
> +
>  static int
>  virDomainMemorySourceDefFormat(virBuffer *buf,
>                                 virDomainMemoryDef *def)
> @@ -27261,6 +27448,10 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
>              return -1;
>      }
>  
> +    for (n = 0; n < def->ncryptos; n++) {
> +        if (virDomainCryptoDefFormat(buf, def->cryptos[n], flags))
> +            return -1;
> +    }
>      if (def->iommu)
>          virDomainIOMMUDefFormat(buf, def->iommu);
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 1404c55053..9062250d60 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -86,6 +86,7 @@ typedef enum {
>      VIR_DOMAIN_DEVICE_IOMMU,
>      VIR_DOMAIN_DEVICE_VSOCK,
>      VIR_DOMAIN_DEVICE_AUDIO,
> +    VIR_DOMAIN_DEVICE_CRYPTO,
>  
>      VIR_DOMAIN_DEVICE_LAST
>  } virDomainDeviceType;
> @@ -118,6 +119,7 @@ struct _virDomainDeviceDef {
>          virDomainIOMMUDef *iommu;
>          virDomainVsockDef *vsock;
>          virDomainAudioDef *audio;
> +        virDomainCryptoDef *crypto;
>      } data;
>  };
>  
> @@ -2858,6 +2860,34 @@ struct _virDomainVsockDef {
>      virDomainVirtioOptions *virtio;
>  };
>  
> +typedef enum {
> +    VIR_DOMAIN_CRYPTO_MODEL_VIRTIO,
> +
> +    VIR_DOMAIN_CRYPTO_MODEL_LAST
> +} virDomainCryptoModel;
> +
> +typedef enum {
> +    VIR_DOMAIN_CRYPTO_TYPE_QEMU,
> +
> +    VIR_DOMAIN_CRYPTO_TYPE_LAST
> +} virDomainCryptoType;
> +
> +typedef enum {
> +    VIR_DOMAIN_CRYPTO_BACKEND_BUILTIN,
> +    VIR_DOMAIN_CRYPTO_BACKEND_LKCF,
> +
> +    VIR_DOMAIN_CRYPTO_BACKEND_LAST
> +} virDomainCryptoBackend;
> +
> +struct _virDomainCryptoDef {
> +    virDomainCryptoModel model;
> +    virDomainCryptoType type;
> +    virDomainCryptoBackend backend;
> +    unsigned int queues;
> +    virDomainDeviceInfo info;
> +    virDomainVirtioOptions *virtio;
> +};
> +
>  struct _virDomainVirtioOptions {
>      virTristateSwitch iommu;
>      virTristateSwitch ats;
> @@ -3023,6 +3053,9 @@ struct _virDomainDef {
>      size_t nsysinfo;
>      virSysinfoDef **sysinfo;
>  
> +    size_t ncryptos;
> +    virDomainCryptoDef **cryptos;
> +
>      /* At maximum 2 TPMs on the domain if a TPM Proxy is present. */
>      size_t ntpms;
>      virDomainTPMDef **tpms;
> @@ -3274,6 +3307,7 @@ struct _virDomainXMLPrivateDataCallbacks {
>      virDomainXMLPrivateDataNewFunc    vcpuNew;
>      virDomainXMLPrivateDataNewFunc    chrSourceNew;
>      virDomainXMLPrivateDataNewFunc    vsockNew;
> +    virDomainXMLPrivateDataNewFunc    cryptoNew;
>      virDomainXMLPrivateDataNewFunc    graphicsNew;
>      virDomainXMLPrivateDataNewFunc    networkNew;
>      virDomainXMLPrivateDataNewFunc    videoNew;
> @@ -3440,6 +3474,9 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainIOMMUDef, virDomainIOMMUDefFree);
>  virDomainVsockDef *virDomainVsockDefNew(virDomainXMLOption *xmlopt);
>  void virDomainVsockDefFree(virDomainVsockDef *vsock);
>  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainVsockDef, virDomainVsockDefFree);
> +virDomainCryptoDef *virDomainCryptoDefNew(virDomainXMLOption *xmlopt);

This function is never defined, only declared here.

> +void virDomainCryptoDefFree(virDomainCryptoDef *crypto);
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainCryptoDef, virDomainCryptoDefFree);
>  void virDomainNetTeamingInfoFree(virDomainNetTeamingInfo *teaming);
>  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainNetTeamingInfo, virDomainNetTeamingInfoFree);
>  void virDomainNetDefFree(virDomainNetDef *def);

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