On 04/14/2018 05:14 PM, John Ferlan wrote:
>
>
> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>> If we are the last one to use pr-manager object we need to remove
>> it and also kill the qemu-pr-helper process.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>> ---
>> src/qemu/qemu_hotplug.c | 38 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 38 insertions(+)
>>
>
> Again, my opinion this is combined too - patch is larger, but everything
> is covered at one time.
>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 43bb910ed6..98e1bf7082 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -3894,6 +3894,34 @@ static bool qemuIsMultiFunctionDevice(virDomainDefPtr def,
>> }
>>
>>
>> +static qemuDomainDiskPRDPtr
>> +qemuDomainDiskNeedRemovePR(virDomainObjPtr vm,
>> + virDomainDiskDefPtr disk)
>> +{
>> + qemuDomainStorageSourcePrivatePtr srcPriv;
>> + size_t i;
>> +
>> + if (!virStoragePRDefIsManaged(disk->src->pr))
>> + return NULL;
>> +
>> + for (i = 0; i < vm->def->ndisks; i++) {
>> + const virDomainDiskDef *domainDisk = vm->def->disks[i];
>> +
>> + if (domainDisk == disk)
>> + continue;
>> +
>> + if (virStoragePRDefIsManaged(domainDisk->src->pr))
>> + break;
>> + }
>> +
>> + if (i != vm->def->ndisks)
>> + return NULL;
>> +
>> + srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
>> + return srcPriv->prd;
>> +}
>> +
>> +
>
> Trying to combine two separate things into one?
>
> Thing 1 -> Remove the object from the domain
> Thing 2 -> Kill the PRD helper *iff* this is the last one using it
>
>> static int
>> qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> @@ -3907,6 +3935,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>> char *drivestr;
>> char *objAlias = NULL;
>> char *encAlias = NULL;
>> + qemuDomainDiskPRDPtr prd = NULL;
>>
>> VIR_DEBUG("Removing disk %s from domain %p %s",
>> disk->info.alias, vm, vm->def->name);
>> @@ -3944,6 +3973,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>> }
>> }
>>
>> + prd = qemuDomainDiskNeedRemovePR(vm, disk);
>> +
>> qemuDomainObjEnterMonitor(driver, vm);
>>
>> qemuMonitorDriveDel(priv->mon, drivestr);
>> @@ -3959,6 +3990,10 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>> ignore_value(qemuMonitorDelObject(priv->mon, encAlias));
>> VIR_FREE(encAlias);
>>
>> + /* If it fails, then so be it - it was a best shot */
>> + if (prd)
>> + ignore_value(qemuMonitorDelObject(priv->mon, prd->alias));
>> +
>
> OK - remove the object if we have one...
>
>> if (disk->src->haveTLS)
>> ignore_value(qemuMonitorDelObject(priv->mon, disk->src->tlsAlias));
>>
>> @@ -3977,6 +4012,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>> }
>> }
>>
>> + if (prd)
>> + qemuProcessKillPRDaemon(vm);
>> +
>
> But we only want to kill if this if there are no other disk's needing
> the pr-helper, right? So we need to have a routine that would be run
> after the disk is really removed. The two phase removal process always
> gets me ;-) - that is Detach vs. Remove and which one is last *after*
> the disk is removed from the list of disks *and* when we know the object
> has been removed from the domain, then we can stop the PR process iff
> this is the last one using it.
Detach is called first and then, when guest OS stopped using the device
(disk in this case) and so did qemu Remove is called. And we have to do
the steps I described in 12/14 but in reverse order:
1) remove the disk (done already, no special handling from this feature
POV),
2) remove pr-manager object (if no other disk still uses it),
3) kill the pr-helper process.
The check from step 2) is done in qemuDomainDiskNeedRemovePR() and on
success (=kill & remove) a non-NULL pointer is returned.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
