On Sun, Mar 12, 2023 at 11:46:36 +0000, Or Ozeri wrote:
>
>
> > -----Original Message-----
> > From: Peter Krempa <pkrempa@xxxxxxxxxx>
> > Sent: Friday, 10 March 2023 11:47
> > To: Or Ozeri <ORO@xxxxxxxxxx>
> > Cc: libvir-list@xxxxxxxxxx; idryomov@xxxxxxxxx; Danny Harnik
> > <DANNYH@xxxxxxxxxx>
> > Subject: [EXTERNAL] Re: [PATCH v1 7/7] qemu: add support for librbd layered
> > encryption
> >
> > > @@ -5210,6 +5216,14 @@
> > qemuDomainValidateStorageSource(virStorageSource *src,
> > > _("librbd encryption is supported only with RBD backed
> > disks"));
> > > return -1;
> > > }
> > > +
> > > + if (src->encryption->nsecrets > 1) {
> > > + if (!virQEMUCapsGet(qemuCaps,
> > QEMU_CAPS_RBD_ENCRYPTION_LAYERING)) {
> > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > > + _("librbd encryption layering is not supported by this
> > QEMU binary"));
> > > + return -1;
> > > + }
> >
> > As noted in previous patch you must here validate that also the disk is not an
> > SD card.
> >
>
> I tried searching the code to understand these questions:
> 1. How to tell that a disk is an SD card?
As you've noted below: disk->bus == VIR_DOMAIN_DISK_BUS_SD
> 2. Why should using multiple secrets be prevented on an SD card disk? And why is a single secret OK?
I mentioned this in my reply of 5/7. You are modifying qemuBuildDriveSourceStr
to ignore the additional secrets. The function is used to format the
disk the old way via '-drive' and this function is at this point used
only for SD cards. I don't think it's viable to add support for the
nested secrets there, thus the need to report
> I could not find an answer to question 2. But I count on your expertise so we can ignore this question.
> The first question however must be answered in order to implement the check you talked about.
> My guess is the answer is (disk->bus == VIR_DOMAIN_DISK_BUS_SD). Is this correct?
Yup.
> But then, you said the check is to be placed inside qemuDomainValidateStorageSource, which has the virStorageSource, but not the parent virDomainDiskDef.
> Do you suggest to extend the signature of qemuDomainValidateStorageSource with an additional "bool isSdDisk"?
Hmm, no I'd rather not pass it down to qemuDomainValidateStorageSource
in the end.
Specifically '-drive' does not accept any of the backingStore image
data, so it requires only validating 'disk->src':
This leaves us with 2 options.
1) Since this is a niche feature and SD cards are also mostly unused
(except for some arm boards). I don't think it could hit too many users.
Thus if librbd+qemu report an (sensible?) error if you don't pass all
the required secrets to unlock the image we can defer this specific case
to pass errors from qemu. This would mean no actual code is required.
2) If the misconfiguration leaves qemu in a broken state for some reason
(e.g. the guest OS reading garbage data rather than an error being
reported) then please re-implement the specific check in a place where
the full disk definition is available.
