On 04/23/2018 09:14 AM, Michal Privoznik wrote:
> For command line we need two things:
>
> 1) -object pr-manager-helper,id=$alias,path=$socketPath
> 2) -drive file.pr-manager=$alias
>
> In -object pr-manager-helper we tell qemu which socket to connect
> to, then in -drive file-pr-manager we just reference the object
> the drive in question should use.
>
> For managed PR helper the alias is always "pr-helper0" and socket
> path "${vm->priv->libDir}/pr-helper0.sock".
>
> It was decided in reviews to previous versions of this patch that
> it should not allocate memory in order to simplify code. This
> promise is not fulfilled though. For instance,
> qemuBuildPRManagerInfoProps() is an offender.
Last paragraph is irrelevant and partially incorrect trivia for above
the ---. I would assume everyone has received review comments that they
may not agree with, but that's what they are review comments. Voicing
frustration in a commit message is probably not a good thing. Good thing
I work at home with usually no one listening 0-).
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/libvirt_private.syms | 2 +
> src/qemu/qemu_alias.c | 18 +++
> src/qemu/qemu_alias.h | 4 +
> src/qemu/qemu_command.c | 134 +++++++++++++++++++++
> src/qemu/qemu_command.h | 5 +
> src/qemu/qemu_domain.c | 22 ++++
> src/qemu/qemu_domain.h | 3 +
> src/qemu/qemu_process.h | 1 +
> src/util/virstoragefile.c | 14 +++
> src/util/virstoragefile.h | 2 +
> .../disk-virtio-scsi-reservations.args | 38 ++++++
> tests/qemuxml2argvtest.c | 4 +
> 12 files changed, 247 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 27c5bb5bce..108b44f6f4 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..9a3a92c82d 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -773,3 +773,21 @@ qemuAliasChardevFromDevAlias(const char *devAlias)
>
> return ret;
> }
> +
> +
> +const char *
> +qemuDomainGetManagedPRAlias(void)
> +{
> + return "pr-helper0";
This works, IDC but you could have gone with
# define QEMU_DOMAIN_MANAGED_PR_ALIAS "pr-helper0"
in src/qemu/qemu_alias.h too and used it that way.
Ironically later on, we have:
#define QEMU_PR_HELPER "/usr/bin/qemu-pr-helper"
and use
VIR_STRDUP(cfg->prHelperName, QEMU_PR_HELPER) < 0)
So why not use the same construct for this alias?
> +}
> +
> +
> +char *
> +qemuDomainGetUnmanagedPRAlias(const virDomainDiskDef *disk)
> +{
> + char *ret;
> +
> + ignore_value(virAsprintf(&ret, "pr-helper-%s", disk->info.alias));
> +
> + return ret;
> +}
[...]
> diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
> index 8c744138ce..8e391c3f0c 100644
> --- a/src/qemu/qemu_alias.h
> +++ b/src/qemu/qemu_alias.h
> @@ -92,4 +92,8 @@ char *qemuAliasTLSObjFromSrcAlias(const char *srcAlias)
> char *qemuAliasChardevFromDevAlias(const char *devAlias)
> ATTRIBUTE_NONNULL(1);
>
> +const char * qemuDomainGetManagedPRAlias(void);
s/* q/*q/
or as I suggest:
# define QEMU_DOMAIN_MANAGED_PR_ALIAS "pr-helper0"
> +
> +char *qemuDomainGetUnmanagedPRAlias(const virDomainDiskDef *disk);
> +
> #endif /* __QEMU_ALIAS_H__*/
[...]
Since the decision was made in other reviews to wait until the last
patch to turn on the capability in the caps*.xml files, the current
.args usage and test config using the DO_TEST macro will have to
suffice; however, when we get to that last patch this will need to
change in order to follow what is being requested of other upstream
posts now too.
With no changes to alias, a reluctant:
Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
a less reluctant one mean change to use the # define construct - I've
even attached a diff that worked for me.
John
>From bf4c63f899eb76413114915aeb9ff6f08dc43cd6 Mon Sep 17 00:00:00 2001
From: John Ferlan <jferlan@xxxxxxxxxx>
Date: Sat, 28 Apr 2018 09:25:40 -0400
Subject: [PATCH 05/15] Use constant for managed pr-helper alias
Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
src/qemu/qemu_alias.c | 7 -------
src/qemu/qemu_alias.h | 2 +-
src/qemu/qemu_command.c | 22 ++++++++++++----------
src/qemu/qemu_domain.c | 5 ++---
4 files changed, 15 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 9a3a92c82d..7afc0efdfe 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -775,13 +775,6 @@ qemuAliasChardevFromDevAlias(const char *devAlias)
}
-const char *
-qemuDomainGetManagedPRAlias(void)
-{
- return "pr-helper0";
-}
-
-
char *
qemuDomainGetUnmanagedPRAlias(const virDomainDiskDef *disk)
{
diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
index 8e391c3f0c..3450910d7d 100644
--- a/src/qemu/qemu_alias.h
+++ b/src/qemu/qemu_alias.h
@@ -92,7 +92,7 @@ char *qemuAliasTLSObjFromSrcAlias(const char *srcAlias)
char *qemuAliasChardevFromDevAlias(const char *devAlias)
ATTRIBUTE_NONNULL(1);
-const char * qemuDomainGetManagedPRAlias(void);
+# define QEMU_DOMAIN_MANAGED_PR_ALIAS "pr-helper0"
char *qemuDomainGetUnmanagedPRAlias(const virDomainDiskDef *disk);
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 22d99e47eb..f8ad77444b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1474,20 +1474,22 @@ static int
qemuBuildDriveSourcePR(virBufferPtr buf,
virDomainDiskDefPtr disk)
{
- char *alias = NULL;
- const char *defaultAlias = NULL;
-
if (!virStoragePRDefIsEnabled(disk->src->pr))
return 0;
- if (virStoragePRDefIsManaged(disk->src->pr))
- defaultAlias = qemuDomainGetManagedPRAlias();
- else if (!(alias = qemuDomainGetUnmanagedPRAlias(disk)))
- return -1;
+ if (virStoragePRDefIsManaged(disk->src->pr)) {
+ virBufferAsprintf(buf, ",file.pr-manager=%s",
+ QEMU_DOMAIN_MANAGED_PR_ALIAS);
+ } else {
+ char *alias = NULL;
+ if (!(alias = qemuDomainGetUnmanagedPRAlias(disk)))
+ return -1;
+
+ virBufferAsprintf(buf, ",file.pr-manager=%s", alias);
+ VIR_FREE(alias);
+ }
- virBufferAsprintf(buf, ",file.pr-manager=%s", alias ? alias : defaultAlias);
- VIR_FREE(alias);
return 0;
}
@@ -9735,7 +9737,7 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
return ret;
if (virStoragePRDefIsManaged(disk->src->pr)) {
- if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
+ if (VIR_STRDUP(alias, QEMU_DOMAIN_MANAGED_PR_ALIAS) < 0)
goto cleanup;
} else {
if (!(alias = qemuDomainGetUnmanagedPRAlias(disk)))
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 53827d5396..6279570611 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11990,15 +11990,14 @@ qemuDomainGetPRSocketPath(virDomainObjPtr vm,
virStoragePRDefPtr pr)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
- const char *defaultAlias = NULL;
char *ret = NULL;
if (!virStoragePRDefIsEnabled(pr))
return NULL;
if (virStoragePRDefIsManaged(pr)) {
- defaultAlias = qemuDomainGetManagedPRAlias();
- ignore_value(virAsprintf(&ret, "%s/%s.sock", priv->libDir, defaultAlias));
+ ignore_value(virAsprintf(&ret, "%s/%s.sock",
+ priv->libDir, QEMU_DOMAIN_MANAGED_PR_ALIAS));
} else {
ignore_value(VIR_STRDUP(ret, pr->path));
}
--
2.13.6
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
