[libvirt] [PATCH v2 10/12] qemu_hotplug: Hotplug of reservations

[Michal Privoznik]

On 03/02/2018 06:55 PM, John Ferlan wrote:
> 
> 
> On 02/21/2018 01:11 PM, Michal Privoznik wrote:
>> Surprisingly, nothing special is happening here. If we are the
>> first to use the managed helper then spawn it. If not, we're
>> almost done.
> 
> But wasn't there a very special reason to start it between fork and
> exec? 

No. If you are referring to previous patch, the very special reason
applies to calling qemuProcessSetupOnePRDaemonHook() which places the
qemu-pr-helper process (well, at this stage it's still just our own
fork) into the namespace of qemu process.

> Does that still hold true? That is why can we start the daemon
> after exec now, but we couldn't before in the previous patch?
> 
> It feels quite "disjoint" to have the unplug in a subsequent patch too.
> Considering them both in one just seems "better", but it's not a deal
> breaker.
> 
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/qemu/qemu_hotplug.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  src/qemu/qemu_process.c | 38 +++++++++++++++++++++-----
>>  src/qemu/qemu_process.h |  7 +++++
>>  3 files changed, 110 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index f28006e3c..2ebb68fbc 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -348,6 +348,58 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>>  }
>>  
>>  
>> +static int
>> +qemuBuildPRDefInfoProps(virDomainObjPtr vm,
> 
> qemuBuild?  Why not qemuDomainAdd?

Dunno. Other functions have the same prefix, e.g.
qemuBuildSecretInfoProps().

> 
>> +                        virDomainDiskDefPtr disk,
>> +                        virJSONValuePtr *prmgrProps,
>> +                        const char **prAlias,
>> +                        const char **prPath)
>> +{
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> +    qemuDomainStorageSourcePrivatePtr srcPriv;
>> +    virJSONValuePtr props = NULL;
>> +    int ret = -1;
>> +
>> +    srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> 
> ohh, umm, in qemuDomainAttachDiskGeneric there's a :
> 
>      srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
>      if (srcPriv) {
>          secinfo = srcPriv->secinfo;
>          encinfo = srcPriv->encinfo;
>      }
> 
> Which makes light dawn that it "was" possible for srcPriv == NULL and
> the "thought" that the subsequent deref is going to fail rather
> spectacularly. See commit '8056721cb' (and a couple of others).
> 
> Although it seems patch 4 and your change to qemuDomainPrepareDiskSource
> to call qemuDomainStorageSourcePrivateNew instead of having it called in
> qemuDomainSecretStorageSourcePrepare would seem to ensure that disk
> source privateData is always allocated now - meaning a number of other
> places where srcPriv is/was checked are no longer necessary.

Yes. There are such places.

> 
> Maybe that one change needs to be "extracted" out to make things
> obvious. Not required, but just thinking and typing thoughts.

Okay, I can try that. Also try removing those unnecessary checks, but as
I was proven many times, touching this part of libvirt code always ends
bad with me.

> 
>> +
>> +    *prmgrProps = NULL;
>> +
>> +    if (priv->prPid != (pid_t) -1 ||
>> +        !srcPriv->prd ||
>> +        !srcPriv->prd->alias)
>> +        return 0;
>> +
>> +    if (virJSONValueObjectCreate(&props,
>> +                                 "s:path", srcPriv->prd->path,
>> +                                 NULL) < 0)
>> +        goto cleanup;
>> +> +    if (qemuProcessSetupOnePRDaemon(vm, disk) < 0)
>> +        goto cleanup;> +
>> +    *prAlias = srcPriv->prd->alias;
>> +    *prPath = srcPriv->prd->path;
>> +    *prmgrProps = props;
>> +    props = NULL;
>> +    ret = 0;
>> + cleanup:
>> +    virJSONValueFree(props);
>> +    return ret;
>> +}
>> +
>> +
>> +static void
>> +qemuDestroyPRDefObject(virDomainObjPtr vm,
> 
> qemuDestroy?  Why not qemuDomainDel?

It's counterpart. qemuBuildPRDef.. qemuDestroyPRDef..

> 
>> +                       const char *alias,
>> +                       const char *path)
>> +{
>> +    if (!alias)
>> +        return;
> 
> BTW: This is where I'd expect to remove the object and then...
> 
>> +
>> +    qemuProcessKillPRDaemons(vm, path, false);
> 
> Just unlink the path...  See some thoughts below...
> 
>> +}
>> +
>> +
>>  /**
>>   * qemuDomainAttachDiskGeneric:
>>   *
>> @@ -365,12 +417,16 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>>      char *devstr = NULL;
>>      char *drivestr = NULL;
>>      char *drivealias = NULL;
>> +    const char *prAlias = NULL;
>> +    const char *prPath = NULL;
>>      bool driveAdded = false;
>>      bool secobjAdded = false;
>>      bool encobjAdded = false;
>> +    bool prmgrAdded = false;
>>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>      virJSONValuePtr secobjProps = NULL;
>>      virJSONValuePtr encobjProps = NULL;
>> +    virJSONValuePtr prmgrProps = NULL;
>>      qemuDomainStorageSourcePrivatePtr srcPriv;
>>      qemuDomainSecretInfoPtr secinfo = NULL;
>>      qemuDomainSecretInfoPtr encinfo = NULL;
>> @@ -403,6 +459,9 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>>                                        disk->info.alias) < 0)
>>          goto error;
>>  
>> +    if (qemuBuildPRDefInfoProps(vm, disk, &prmgrProps, &prAlias, &prPath) < 0)
>> +        goto error;
>> +
>>      if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps)))
>>          goto error;
>>  
>> @@ -435,6 +494,15 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>>          encobjAdded = true;
>>      }
>>  
>> +    if (prmgrProps) {
>> +        rv = qemuMonitorAddObject(priv->mon, "pr-manager-helper", prAlias,
>> +                                  prmgrProps);
>> +        prmgrProps = NULL; /* qemuMonitorAddObject consumes */
>> +        if (rv < 0)
>> +            goto exit_monitor;
>> +        prmgrAdded = true;
>> +    }
>> +
> 
> So for one of the managed modes (as noted much earlier) we could be
> creating a object with a duplicated alias - does that work? I thought
> id= has to be unique.

How come? The first thing that qemuBuildPRDefInfoProps() does is it
checks 'priv->prPid != (pid_t) -1'. The fact that this pid is not -1
means that there is a pr-helper process already running.

> 
>>      if (qemuMonitorAddDrive(priv->mon, drivestr) < 0)
>>          goto exit_monitor;
>>      driveAdded = true;
>> @@ -455,6 +523,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>>   cleanup:
>>      virJSONValueFree(secobjProps);
>>      virJSONValueFree(encobjProps);
>> +    virJSONValueFree(prmgrProps);
>>      qemuDomainSecretDiskDestroy(disk);
>>      VIR_FREE(devstr);
>>      VIR_FREE(drivestr);
>> @@ -472,6 +541,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>>          ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias));
>>      if (encobjAdded)
>>          ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias));
>> +    if (prmgrAdded)
>> +        ignore_value(qemuMonitorDelObject(priv->mon, prAlias));
>>      if (qemuDomainObjExitMonitor(driver, vm) < 0)
>>          ret = -2;
>>      virErrorRestore(&orig_err);
>> @@ -481,6 +552,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>>   error:
>>      qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src);
>>      ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true));
>> +    qemuDestroyPRDefObject(vm, prAlias, prPath);
> 
> oh, I see, you're mixing the way TLS object tear down occurred and how
> other objects happen.  If you're going to mimic TLS, then change
> qemuBuildPRDefInfoProps to be something like qemuDomainAddPRDefObject
> which would do the EnterMonitorAsync, Props mgmt, AddObject, and
> ExitMonitor.
> 
> That way qemuDestroyPRDefObject changes to qemuDomainDelPRDefObject
> which would handle the failure path.
> 
> Then I believe you don't have to pass around prAlias and prPath since
> they'd be "obtainable".

Actually, I'm mimicing qemuBuildSecretInfoProps().

> 
>>      goto cleanup;
>>  }
>>  
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 33e0ad30c..3a914b6c6 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -2556,16 +2556,40 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver,
>>  }
>>  
>>  
> 
> This subsequent hunk feels like it could have gone in for the previous
> patch.  Or at the very least some function that would unlink the socket
> path for each of the sockets in ndisks that were created.  Then that
> single unlink API gets exposed.

Well, the only socket we can unlink is for the managed pr-helper.
Otherwise we might be unlinking a socket that is still active. And since
I need to use those static functions only in this patch I'm exposing
them here.

> 
>> -static void
>> -qemuProcessKillPRDaemons(virDomainObjPtr vm)
>> +void
>> +qemuProcessKillPRDaemons(virDomainObjPtr vm,
>> +                         const char *socketPath,
>> +                         bool force)
> 
> The only time force == true is when socketPath == NULL... and that's
> only in the shutdown path.  When socketPath != NULL, force == false, and
> we're going to unplug the socket, right?

Yes.

> 
> Perhaps this would be cleaner if only @socketPath was in play. If NULL,
> then walk the ndisks looking for paths that you'll unlink first before
> killing the daemon.
> 
> I actually think having a separate unlink the socket would be just fine.
> Does it really matter if it's the "last" one to stop the helper daemon?

I think it does, because if we did not stop it I can see people
complaining immediately. We would be leaving a SUID process around for
longer than needed. Which increases the attack surface (the process has
a socket which is writable to UID:GID of the corresponding qemu process
and can do RAWIO operations).

Michal