Thanks for reviewing all of my patches!<div style="font-size: 12.8px;">I'm fine with you making any of the changes you suggested.</div><div style="font-size: 12.8px;"> </div><div style="font-size: 12.8px;">So the only change I need to make is "specify what's happening in the storage driver"?</div><div style="font-size: 12.8px;">Can you elaborate what do you mean by that?</div><div style="font-size: 12.8px;"> </div><div style="font-size: 12.8px;">I can add something like:</div><div style="font-size: 12.8px;">For librbd engine, the encryption happens inside the librbd storage driver, so block read/write requests coming in from the hypervisor (qemu) are plaintext,</div><div style="font-size: 12.8px;">but encrypted by the storage driver before being persisted.</div><div style="font-size: 12.8px;"> </div><div style="font-size: 12.8px;">Is this the kind of thing you were thinking about?</div><div style="font-size: 12.8px;"> </div><div>-----"Peter Krempa" <<a href="mailto:pkrempa@redhat.com" target="_blank" rel="noopener noreferrer">pkrempa@redhat.com</a>> wrote: -----</div> >To: "Or Ozeri" <<a href="mailto:oro@il.ibm.com" target="_blank" rel="noopener noreferrer">oro@il.ibm.com</a>> >From: "Peter Krempa" <<a href="mailto:pkrempa@redhat.com" target="_blank" rel="noopener noreferrer">pkrempa@redhat.com</a>> >Date: 10/21/2021 02:16PM >Cc: <a href="mailto:libvir-list@redhat.com" target="_blank" rel="noopener noreferrer">libvir-list@redhat.com</a>, <a href="mailto:idryomov@gmail.com" target="_blank" rel="noopener noreferrer">idryomov@gmail.com</a>, ><a href="mailto:to.my.trociny@gmail.com" target="_blank" rel="noopener noreferrer">to.my.trociny@gmail.com</a>, <a href="mailto:dannyh@il.ibm.com" target="_blank" rel="noopener noreferrer">dannyh@il.ibm.com</a> >Subject: [EXTERNAL] Re: [PATCH v4 4/5] qemu: add librbd encryption >engine > >On Thu, Oct 07, 2021 at 14:21:20 -0500, Or Ozeri wrote: >> rbd encryption is new in qemu 6.1.0. >> This commit adds a new encryption engine property which >> allows the user to use this new encryption engine. >> >> Signed-off-by: Or Ozeri <<a href="mailto:oro@il.ibm.com" target="_blank" rel="noopener noreferrer">oro@il.ibm.com</a>> >> --- >> docs/formatstorageencryption.html.in | 7 +- >> docs/schemas/storagecommon.rng | 1 + >> src/conf/storage_encryption_conf.c | 2 +- >> src/conf/storage_encryption_conf.h | 1 + >> src/qemu/qemu_block.c | 26 +++++++ >> src/qemu/qemu_domain.c | 34 +++++++++ >> ...sk-network-rbd-encryption.x86_64-6.0.0.err | 1 + >> ...-network-rbd-encryption.x86_64-latest.args | 45 ++++++++++++ >> .../disk-network-rbd-encryption.xml | 63 >+++++++++++++++++ >> tests/qemuxml2argvtest.c | 2 + >> ...k-network-rbd-encryption.x86_64-latest.xml | 70 >+++++++++++++++++++ >> tests/qemuxml2xmltest.c | 1 + >> 12 files changed, 251 insertions(+), 2 deletions(-) >> create mode 100644 >tests/qemuxml2argvdata/disk-network-rbd-encryption.x86_64-6.0.0.err >> create mode 100644 >tests/qemuxml2argvdata/disk-network-rbd-encryption.x86_64-latest.args >> create mode 100644 >tests/qemuxml2argvdata/disk-network-rbd-encryption.xml >> create mode 100644 >tests/qemuxml2xmloutdata/disk-network-rbd-encryption.x86_64-latest.xm >l >> >> diff --git a/docs/formatstorageencryption.html.in >b/docs/formatstorageencryption.html.in >> index 178fcd0d7c..02ee8f8ca3 100644 >> --- a/docs/formatstorageencryption.html.in >> +++ b/docs/formatstorageencryption.html.in >> @@ -27,7 +27,12 @@ >> The <code>encryption</code> tag supports an optional ><code>engine</code> >> tag, which allows selecting which component actually handles >> the encryption. Currently defined values of ><code>engine</code> are >> - <code>qemu</code>. >> + <code>qemu</code> and <code>librbd</code>. >> + Both <code>qemu</code> and <code>librbd</code> require using >the qemu driver. >> + The <code>librbd</code> engine requires qemu version >= >6.1.0, >> + and is only applicable for RBD network disks. >> + If the engine tag is not specified, the <code>qemu</code> >engine will be >> + used by default (assuming the qemu driver is used). > >Okay, this is slightly better but it doesn't specify what's happening >in >the storage driver. > >> >> >> The <code>encryption</code> tag can currently contain a >sequence of > >[...] > > >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> index 354f65c6d5..13869dd79b 100644 >> --- a/src/qemu/qemu_domain.c >> +++ b/src/qemu/qemu_domain.c >> @@ -4814,6 +4814,40 @@ >qemuDomainValidateStorageSource(virStorageSource *src, >> if (src->encryption) { >> switch (src->encryption->engine) { >> case VIR_STORAGE_ENCRYPTION_ENGINE_QEMU: >> + switch ((virStorageEncryptionFormatType) >src->encryption->format) { >> + case VIR_STORAGE_ENCRYPTION_FORMAT_LUKS: >> + case VIR_STORAGE_ENCRYPTION_FORMAT_QCOW: >> + break; >> + >> + case VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT: >> + case VIR_STORAGE_ENCRYPTION_FORMAT_LAST: >> + default: >> + >virReportEnumRangeError(virStorageEncryptionFormatType, >> + >src->encryption->format); >> + return -1; > >This here is okay, because both are basically impossible. > >> + } >> + >> + break; >> + case VIR_STORAGE_ENCRYPTION_ENGINE_LIBRBD: >> + if (!virQEMUCapsGet(qemuCaps, >QEMU_CAPS_RBD_ENCRYPTION)) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >"%s", >> + _("librbd encryption is not >supported by this QEMU binary")); >> + return -1; >> + } >> + >> + switch ((virStorageEncryptionFormatType) >src->encryption->format) { >> + case VIR_STORAGE_ENCRYPTION_FORMAT_LUKS: >> + break; >> + >> + case VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT: >> + case VIR_STORAGE_ENCRYPTION_FORMAT_QCOW: >> + case VIR_STORAGE_ENCRYPTION_FORMAT_LAST: >> + default: >> + >virReportEnumRangeError(virStorageEncryptionFormatType, >> + >src->encryption->format); > >This creates an error message which is not very informative. >Specifically for VIR_STORAGE_ENCRYPTION_FORMAT_QCOW which is a >legitimate configuration we need a proper error message. > >> + return -1; >> + } >> + >> break; >> case VIR_STORAGE_ENCRYPTION_ENGINE_DEFAULT: >> case VIR_STORAGE_ENCRYPTION_ENGINE_LAST: > >[...] > >The test input files are okay. The output files will need some >tweaking >after recent changes. I think adding the error message is trivial >enough >so that I'll do it before pushing if you are okay with it. > >Reviewed-by: Peter Krempa <<a href="mailto:pkrempa@redhat.com" target="_blank" rel="noopener noreferrer">pkrempa@redhat.com</a>> > >