8:43 a.m.
On 04/16/2018 10:57 AM, Michal Privoznik wrote:
On 04/14/2018 05:14 PM, John Ferlan wrote:
>
>
> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>> When attaching a disk that requires pr-manager we might need to
>> plug the pr-manager object too.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> src/qemu/qemu_hotplug.c | 41 +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 41 insertions(+)
>>
>
> My opinion - combine w/ previous patch.
>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 468153c79c..43bb910ed6 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -391,6 +391,29 @@ qemuDomainMaybeStartPRDaemon(virDomainObjPtr vm,
>> }
>>
>>
>> +static int
>> +qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm,
>> + qemuDomainDiskPRDPtr prd,
>> + virJSONValuePtr *propsret)
>> +{
>> + size_t i;
>> +
>> + *propsret = NULL;
>> +
>> + for (i = 0; i < vm->def->ndisks; i++) {
>> + const virDomainDiskDef *domainDisk = vm->def->disks[i];
>> +
>> + if (virStoragePRDefIsManaged(domainDisk->src->pr)) {
>> + /* qemu-pr-helper should be already started because
>> + * another disk in domain requires it. */
>> + return 0;
>> + }
>> + }
>
> I don't understand the need for the above hunk - that was done in
> patch11 wasn't it? This is why I like things combined - going back and
> forth and changing logic as patches are developed means something may be
> missed.
Okay the comment might be misleading. It should be s/started/added/.
Anwyay, when adding a disk that has PR configured we need to:
1) start pr-helper process (if we are the ones managing it),
2) construct JSON for pr-manager object,
3) add pr-mananager object on the monitor,
4) finally, add disk on the monitor.
And we need to do it in this order, otherwise qemu might try to connect
to not yet running process. Now, obviously, since there can be only one
managed pr-helper process per domain, there can be only one pr-manager
object. Therefore, when somebody is trying to hotplug a disk with PR
enabled & managed, but there already is a disk like that: step 1) turns
into no-op (see previous patch), and we also need steps 2) and 3) to
turn into no-op because the object is already in qemu. Trying to add it
again would result in error.
qemuDomainMaybeStartPRDaemon which is a few lines above here and in the
previous patch is what handled potentially starting the PR Daemon.
Once I get here, then I'm running through the same list, doing the same
check, and then deciding whether to create the object, but that would
seem to be the task of the previous patch, but they're intertwined
dependencies, so IMO, one patch.
Since adding the ",file.pr-manager=%s" is part of qemuBuildDriveDevStr,
it's not like anything done in this patch that's doing anything that
couldn't be combined with the previous one.
>
>> +
>> + return qemuBuildPRManagerInfoProps(prd, propsret);
>> +}
>> +
>> +
>> /**
>> * qemuDomainAttachDiskGeneric:
>> *
>> @@ -411,13 +434,16 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>> bool driveAdded = false;
>> bool secobjAdded = false;
>> bool encobjAdded = false;
>> + bool prmgrAdded = false;
>> bool prdStarted = false;
>> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>> virJSONValuePtr secobjProps = NULL;
>> virJSONValuePtr encobjProps = NULL;
>> + virJSONValuePtr prmgrProps = NULL;
>> qemuDomainStorageSourcePrivatePtr srcPriv;
>> qemuDomainSecretInfoPtr secinfo = NULL;
>> qemuDomainSecretInfoPtr encinfo = NULL;
>> + qemuDomainDiskPRDPtr prd = NULL;
>>
>> if (qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, false) < 0)
>> goto cleanup;
>> @@ -437,6 +463,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>> if (srcPriv) {
>> secinfo = srcPriv->secinfo;
>> encinfo = srcPriv->encinfo;
>> + prd = srcPriv->prd;
>
> As noted much earlier - the private data isn't needed here as the prd is
> in the disk storage definition.
>
>> }
>>
>> if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES)
{
>> @@ -447,6 +474,9 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>> if (encinfo && qemuBuildSecretInfoProps(encinfo, &encobjProps)
< 0)
>> goto error;
>>
>> + if (qemuMaybeBuildPRManagerInfoProps(vm, prd, &prmgrProps) < 0)
>> + goto error;
>> +
>
> This should just call qemuBuildPRManagerInfoProps directly since it
> already checks if !prd || !prd->alias
No, because we might end up trying to add pr-manager object that is
already there in qemu which would fail and so would whole hotplug opertaion.
Of course the comment came from the same notion that running through
ndisks again isn't really necessary and my continued thought that the 11
and 12 should be combined.
John