Re: [libvirt] [PATCH v4 13/14] qemu_hotplug: Hotunplug of reservations

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(a)redhat.com&gt; > --- > 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.