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

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