Re: [libvirt] [PATCH v2 04/12] qemu: Generate alias and socket path for pr-helper
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(a)redhat.com>
> ---
> 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