Re: [libvirt] [PATCH] conf: Parse virtio-crypto in the domain XML

On Mon, Jan 09, 2017 at 10:15:25AM +0800, Longpeng(Mike) wrote: > As virtio-crypto has been supported in QEMU 2.9 and > the frontend driver has been merged in linux 4.10, > so it's necessary to support virtio-crypto in domain > XML. > > This patch parse the domain XML with virtio-crypto > support, the virtio-crypto XML looks like this: > > <crypto model='virtio'> > <backend type='builtin' queues='2'/> > </crypto> > > Signed-off-by: Longpeng(Mike) <longpeng2(a)huawei.com&gt; > --- > src/conf/domain_conf.c | 212 +++++++++++++++++++++++++++++++++++++++++ > src/conf/domain_conf.h | 32 +++++++ > src/qemu/qemu_alias.c | 20 ++++ > src/qemu/qemu_alias.h | 4 + > src/qemu/qemu_capabilities.c | 4 + > src/qemu/qemu_capabilities.h | 2 + > src/qemu/qemu_command.c | 129 +++++++++++++++++++++++++ > src/qemu/qemu_command.h | 4 + > src/qemu/qemu_domain_address.c | 17 ++++ > src/qemu/qemu_driver.c | 6 ++ > src/qemu/qemu_hotplug.c | 1 + > 11 files changed, 431 insertions(+) > No tests, no documentation, no schema => no acks. The patch should be split into at least two parts (conf, schema, qemuxml2xmltest and docs in one; then hypervisor functionality and tests in the second one).

Also I see no hunk adding anything to qemuDomainAssignDevicePCISlots() or similar.

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 9f7b906..fcfccd5 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -237,6 +237,7 @@ VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST, > "memballoon", > "nvram", > "rng", > + "crypto", Why are you adding this in a random place rather than at the end? That could be fixed in most of the hunks.

> @@ -21590,6 +21749,51 @@ virDomainRNGDefFree(virDomainRNGDefPtr def) > > > static int > +virDomainCryptoDefFormat(virBufferPtr buf, > + virDomainCryptoDefPtr def, > + unsigned int flags) > +{ > + const char *model = virDomainCryptoModelTypeToString(def->model); > + const char *backend = virDomainCryptoBackendTypeToString(def->backend); > + > + virBufferAsprintf(buf, "<crypto model='%s'>\n", model); > + virBufferAdjustIndent(buf, 2); > + virBufferAsprintf(buf, "<backend type='%s'", backend); > + > + switch ((virDomainCryptoBackend) def->backend) { > + case VIR_DOMAIN_CRYPTO_BACKEND_BUILTIN: > + if (def->queues) > + virBufferAsprintf(buf, " queues='%u'", def->queues); > + > + virBufferAddLit(buf, "/>\n"); > + break; > + > + case VIR_DOMAIN_CRYPTO_BACKEND_LAST: > + break; > + } > + > + if (virDomainDeviceInfoNeedsFormat(&def->info, flags)) { It should always have an address, this is not an old device with back-compat problems.

> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 2c0b29d..b1b718b 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -337,6 +337,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, > "drive-detect-zeroes", > > "tls-creds-x509", /* 230 */ > + "cryptodev-backend-builtin", > + "virtio-crypto", > ); > > > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h > index affb639..98cb817 100644 > --- a/src/qemu/qemu_capabilities.h > +++ b/src/qemu/qemu_capabilities.h > @@ -370,6 +370,8 @@ typedef enum { > > /* 230 */ > QEMU_CAPS_OBJECT_TLS_CREDS_X509, /* -object tls-creds-x509 */ > + QEMU_CAPS_OBJECT_CRYPTO_BUILTIN, > + QEMU_CAPS_DEVICE_VIRTIO_CRYPTO, > Base your patch on master, not on some old, private, branch. Also see how every capability has a nice comment even if it's very trivial. It can help sometimes.

I haven't tested it or read it thoroughly, but the rest looks reasonable.

Martin