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@xxxxxxxxxx>
>> ---
>> 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.
>
>
>> 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.
>
> 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. 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
> 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,
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.
>
> diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
>> index 8c744138ce..91e0a9c893 100644
>> --- a/src/qemu/qemu_alias.h
>> +++ b/src/qemu/qemu_alias.h
>> @@ -92,4 +92,6 @@ char *qemuAliasTLSObjFromSrcAlias(const char *srcAlias)
>> char *qemuAliasChardevFromDevAlias(const char *devAlias)
>> ATTRIBUTE_NONNULL(1);
>>
>> +char * qemuDomainGetManagedPRAlias(void);
>> +
>> #endif /* __QEMU_ALIAS_H__*/
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 5a7b5f8417..361a80be84 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -982,6 +982,18 @@ qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr *secinfo)
>> }
>>
>>
>> +static void
>> +qemuDomainDiskPRDFree(qemuDomainDiskPRDPtr prd)
>> +{
>> + if (!prd)
>> + return;
>> +
>> + VIR_FREE(prd->alias);
>> + VIR_FREE(prd->path);
>> + VIR_FREE(prd);
>> +}
>> +
>> +
>> static virClassPtr qemuDomainDiskPrivateClass;
>> static void qemuDomainDiskPrivateDispose(void *obj);
>>
>> @@ -1062,6 +1074,7 @@ qemuDomainStorageSourcePrivateDispose(void *obj)
>>
>> qemuDomainSecretInfoFree(&priv->secinfo);
>> qemuDomainSecretInfoFree(&priv->encinfo);
>> + qemuDomainDiskPRDFree(priv->prd);
>> }
>>
>>
>> @@ -1473,9 +1486,6 @@ qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivatePtr priv,
>> if (!hasAuth && !hasEnc)
>> return 0;
>>
>> - if (!(src->privateData = qemuDomainStorageSourcePrivateNew()))
>> - return -1;
>> -
>> srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
>>
>> if (hasAuth) {
>> @@ -11925,11 +11935,79 @@ qemuDomainPrepareDiskCachemode(virDomainDiskDefPtr disk)
>> }
>>
>>
>> +static int
>> +qemuDomainPrepareDiskPRD(qemuDomainObjPrivatePtr priv,
>> + virDomainDiskDefPtr disk)
>> +{
>> + qemuDomainStorageSourcePrivatePtr srcPriv;
>> + virStoragePRDefPtr prd = disk->src->pr;
>> + char *prAlias = NULL;
>> + char *prPath = NULL;
>> + int ret = -1;
>> +
>> + if (!virStoragePRDefIsEnabled(prd))
>> + return 0;
>> +
>> + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("reservations not supported with this QEMU binary"));
>> + return ret;
>> + }
>> +
>> + if (!virStorageSourceIsLocalStorage(disk->src)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("reservations supported only for local storage"));
>> + return ret;
>> + }
>
> Not only local, but local and LUN ... Still this check probably should
> have gone into the Validate code if anything.
I don't think it can go there. For IsLocalStorage() to return valid
results virDomainDiskTranslateSourcePool() needs to be called first. And
it is done so only when starting domain. Not for Validate() callback.
And technically, at this point whether disk is LUN or not is checked in
virDomainDiskDefValidate(). So the control would not even get here. But
okay, I will change the error message.
>
>> +
>> + srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
>> +
>> + /* Managed PR means there's one pr-manager object per domain
>> + * and the pr-helper process is spawned and managed by
>> + * libvirt.
>> + * If PR is unmanaged there's one pr-manager object per disk
>> + * (in general each disk can have its own pr-manager) and
>> + * it's user's responsibility to start the pr-helper process.
>> + */
>> + if (virStoragePRDefIsManaged(prd)) {
>> + /* Generate PR helper socket path & alias that are same
>> + * for each disk in the domain. */
>> +
>> + if (!(prAlias = qemuDomainGetManagedPRAlias()))
>
> just make it a straight VIR_STRDUP("pr-helper0")
No. Having alias generation at single location is very advantageous IMO.
Libvirt's full of small wrappers over functions. And it this case it
even makes sense.
>
> or some sort of #define for VIRT_PR_MANAGED_ALIAS that's usable by
> qemuProcessKillPRDaemon... You could go really obscure by using
> "pr-helper" as the defined value and then using that in all your various
> printf's.
Then I'd have to modify qemuDomainDiskPRDFree() so that for instance it
does:
if (STRNEQ(prd->alias, QEMU_PR_MANAGER_ALIAS))
VIR_FREE(prd->alias);
VIR_FREE(prd->path);
...
which I don't quite like (to say the least).
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
