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

[John Ferlan]

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 at redhat.com>
> ---
>  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.

John

>      qemuDomainReleaseDeviceAddress(vm, &disk->info, src);
>  
>      if (qemuSecurityRestoreDiskLabel(driver, vm, disk) < 0)
>