Re: [libvirt] [PATCH v4 04/14] qemu: Generate alias and socket path for pr-helper

On 04/16/2018 10:56 AM, Michal Privoznik wrote: > On 04/13/2018 10:51 PM, John Ferlan wrote: >> >> >> On 04/10/2018 10:58 AM, Michal Privoznik wrote: >>> While we're not generating the command line just yet (look for >>> the next commits), we can generate the alias for pr-manager. >>> A domain can have up to one managed pr-manager (in which case >>> socket path is decided by libvirt and pr-helper is spawned by >>> libvirt too), but up to ndisks of unmanaged ones (one per disk >>> basically). In case of the former we can generate a simple alias >>> and be sure it'll not conflict. But in case of the latter to >> >> Well if it's only one, then it had better not conflict (IOW: after the >> and is unnecessary because there is check). >> >>> avoid any conflicts let's generate alias that's based on >>> something unique - like disk target. >>> >> >> Make sure this commit message follows whatever happens in this patch... >> >>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >>> --- >>> src/libvirt_private.syms | 2 ++ >>> src/qemu/qemu_alias.c | 11 ++++++ >>> src/qemu/qemu_alias.h | 2 ++ >>> src/qemu/qemu_domain.c | 87 +++++++++++++++++++++++++++++++++++++++++++++-- >>> src/qemu/qemu_domain.h | 10 ++++++ >>> src/util/virstoragefile.c | 15 ++++++++ >>> src/util/virstoragefile.h | 2 ++ >>> 7 files changed, 126 insertions(+), 3 deletions(-) >>> >> >> This patch does two things - one is just the pure alias/path for the >> pr-helper for the active domain. The second is copy that same >> information to the private storage source, which I'm not sure (yet) why >> it's necessary since both can be regenerated as needed based on fairly >> static data as opposed to secinfo and encinfo which get filled in with >> information only available during Prepare (the interaction with the >> secret driver and the need for the @conn pointer) that wasn't available >> during launch/command line building. Although some of that has changed >> with more recent changes to be able to get a secret conn "on the fly". >> Anyway, I digress. >> >> The point being - please note why you're also saving something in the >> private storage source area... The 'path' is already present in the >> domain XML (both active and inactive) and the 'alias' could be saved in >> the active output if you tweaked virStoragePRDefFormat to match what >> virDomainDeviceInfoFormat does. > > Okay. I will put it in the commit message. I thought we've discussed > this earlier. I rather put and info into status XML even though it might > look useless right now than having to write some crazy backcompat code > later. > I don't think this is the same as @channelTargetDir (as you note in the next response). The problem there was a reliance on a name that ended up being too long and yes, it was a mess. I dunno - I think I've tried the future proofing things too and was told it was unnecessary.

Still in order to "provide insight" into my why... Here you have a @path that's essentially static outside of the commonly used priv->libDir and an @alias that is static when managed. When unmanaged, the @path is provided by a consumer and the @alias is essentially a static prefix followed by a commonly used source destination alias. So nothing so far that would need to be saved in two places.

Since @alias is an "internal" libvirt->qemu construct we wouldn't have the same problem as some "external" construct changing our definition. Unlike perhaps _qemuDomainVcpuPrivate where the alias perhaps changes based on ordering, insert, remove, etc. (I didn't look, but made the assumption). For PR there is one and it's not going to change. Even if it did, we would always know the former format to search on before using a newer format.

In the long run, since there would be only one the PR alias belongs in the _qemuDomainObjPrivate further removing the need to have anything private. As noted somewhere during my review the existing need for a private structure for secrets could now be reworked/removed because the code now can generate the @conn on the fly rather than needing the @conn from the original connection in order to lookup the secret. Depends on if we want an error during command line generation or during the prepare phase. >> >> >>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >>> index a376e3bb0d..5b7b5514a2 100644 >>> --- a/src/libvirt_private.syms >>> +++ b/src/libvirt_private.syms >>> @@ -2799,7 +2799,9 @@ virStorageNetHostTransportTypeToString; >>> virStorageNetProtocolTypeToString; >>> virStoragePRDefFormat; >>> virStoragePRDefFree; >>> +virStoragePRDefIsEnabled; >>> virStoragePRDefIsEqual; >>> +virStoragePRDefIsManaged; >>> virStoragePRDefParseXML; >>> virStorageSourceBackingStoreClear; >>> virStorageSourceClear; >>> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c >>> index 95d1e0370a..6db3da27a8 100644 >>> --- a/src/qemu/qemu_alias.c >>> +++ b/src/qemu/qemu_alias.c >>> @@ -773,3 +773,14 @@ qemuAliasChardevFromDevAlias(const char *devAlias) >>> >>> return ret; >>> } >>> + >>> + >>> +char * >>> +qemuDomainGetManagedPRAlias(void) >>> +{ >>> + char *alias; >>> + >>> + ignore_value(VIR_STRDUP(alias, "pr-helper0")); >>> + >>> + return alias; >>> +} >> >> I don't really see a purpose for this function. If it were to survive, >> then it could take a parameter "const char *srcalias" and create/return >> the alias from that based on what's in qemuDomainPrepareDiskPRD. > > The purpose is to generate alias at only one place instead of several > virAsprintf-s scattered all around the code. > ug, sure this is similar to qemuDomainGetMasterKeyAlias or qemuAssignDeviceWatchdogAlias. Since there can only ever be one managed PR per domain, then scattered is I think limited to two places - start and stop.

>> >> Eventually when the qemuProcessKillPRDaemon is created, it doesn't >> necessarily distinguish between managed and unmanaged... > > Well, unmanaged helper should not have its socket in libvirt private > path. Under any circumstances. The KillPRDaemon doesn't distinguish between managed/unmanaged, but callers do - perhaps wasn't as clear yet when I noted that... > And what qemuProcessKillPRDaemon does is: > 1) it constructs PID file path > 2) and kills any pr-helper that might had been left behind. > > I find this approach more robust than plain "check if there is a disk > with pr-helper and if so kill it" because if libvirt is ever split brained > Jumping forward 6 patches. Maybe it would have helped to have all the logic that would create a PR process "in place". Then add the disk logic. The two concepts are intertwined and related, but it is possible to separate too.

>> Still having it >> fail because it couldn't strdup what amounts to be a static string >> doesn't make much sense. > > Well, there are couple of reasons for stdup()-ing a static string: > a) it simplifies the code, e.g. qemuDomainDiskPRDFree() doesn't have to > care if alias is a static string or dynamically allocated one, I don't think I was advocating usage of a static string to be saved in the ->alias field. I was merely commenting on the need to have a helper to perform the strdup of a static string that could be some #define somewhere.... IOW, later on it's: if (VIR_STRDUP(prAlias, VIRT_PR_MANAGED_ALIAS) < 0) instead of if (!(prAlias = qemuDomainGetManagedPRAlias())) > b) higher memory consumption (by 11 bytes btw) is only for a short > period of time (when constructing PID file path). Although, the alias is > kept around for entire time domain is running. > > And if libvirt is unable to allocate 11 bytes then you're in much bigger > trouble anyways. 11 here, 11 there, it all adds up. But no, it's not the 11 or probably more like 16, 32, 64, etc. whatever the underlying code would consider it's smallest hunk. Still if you had 10 managed LUNs, that's a lot of replicated data.