On 03/02/2018 02:59 AM, John Ferlan wrote:
>
>
> On 02/21/2018 01:11 PM, 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
>> avoid any conflicts let's generate alias that's based on
>> something unique - like disk target.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>> ---
>> src/libvirt_private.syms | 2 ++
>> src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++++++--
>> src/qemu/qemu_domain.h | 10 ++++++
>> src/util/virstoragefile.c | 15 +++++++++
>> src/util/virstoragefile.h | 2 ++
>> 5 files changed, 106 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index de8974d66..dffc4c30e 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -11538,17 +11548,81 @@ qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr driver,
>> }
>>
>>
>> +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"));
>> + goto cleanup;
>> + }
>> +
>> + if (!virStorageSourceIsLocalStorage(disk->src)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("reservations supported only for local storage"));
>> + goto cleanup;
>> + }
>
> Ironically the above two could return -1 directly...
>
>> +
>> + srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
>> +
>> + if (virStoragePRDefIsManaged(prd)) {
>> + /* Generate PR helper socket path & alias that are same
>> + * for each disk in the domain. */
>> +
>> + if (VIR_STRDUP(prAlias, "pr-helper0") < 0)
>> + return -1;
>> +
>> + if (virAsprintf(&prPath, "%s/pr-helper0.sock", priv->libDir) < 0)
>> + return -1;
>
> Leaks prAlias
Ah, good catch.
>
> BTW: Whatever "managed" ends up being needs to be described somewhere
> either prior to this or after this, but knowing how the connection
> between libvirt and qemu's process is going to happen will be really
> helpful... Something that I would think would be described in patch 1,
> but again that's just me. This whole managed thing is really confusing
> the way it's written. May make sense to you, but it doesn't make sense
> to me, just saying.
Fair enough. I'll add something.
>
>> +
>> + } else {
>> + if (virAsprintf(&prAlias, "pr-helper-%s", disk->dst) < 0)
>> + return -1;
>
> Or using the 'disk->info.alias' like other consumers.
Okay. I though that disk->dst would be easier to find on cmd line but on
the other hand, grepping cmd line for disk->info.alias shows every
argument that has something to do with the disk.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
