10:44 a.m.
On Mon, May 14, 2018 at 16:27:17 -0400, John Ferlan wrote:
On 05/14/2018 06:41 AM, Peter Krempa wrote:
> Rather than always checking which path to use pre-assign it when
> preparing storage source.
>
> This reduces the need to pass 'vm' around too much. For later use the
> path can be retrieved from the status XML.
>
> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
> ---
> src/qemu/qemu_command.c | 18 +++++-------------
> src/qemu/qemu_command.h | 3 +--
> src/qemu/qemu_domain.c | 35 ++++++++++++++++++++++-------------
> src/qemu/qemu_domain.h | 3 +--
> src/qemu/qemu_hotplug.c | 2 +-
> src/qemu/qemu_process.c | 2 +-
> 6 files changed, 31 insertions(+), 32 deletions(-)
>
Strange this patch got posted twice once w/ your own CC and once without.
Yes, my mailserver rejected sending of the next patch so I've retried it
manually without the CC and messed up selecting which ones I should
resend.
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2bdba7734a..7df9979cb2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9667,7 +9667,6 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
>
> /**
> * qemuBuildPRManagerInfoProps:
> - * @vm: domain object
> * @disk: disk definition
> * @propsret: Returns JSON object containing properties of the pr-manager-helper
object
> * @aliasret: alias of the pr-manager-helper object
> @@ -9678,12 +9677,10 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
> * -1 on failure (with error message set).
> */
> int
> -qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
> - const virDomainDiskDef *disk,
> +qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
> virJSONValuePtr *propsret,
> char **aliasret)
> {
> - char *socketPath = NULL;
> char *alias = NULL;
> int ret = -1;
>
> @@ -9693,9 +9690,6 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
> if (!disk->src->pr)
> return 0;
>
> - if (!(socketPath = qemuDomainGetPRSocketPath(vm, disk->src->pr)))
> - return ret;
> -
> if (virStoragePRDefIsManaged(disk->src->pr)) {
> if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
> goto cleanup;
> @@ -9705,7 +9699,7 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
> }
>
> if (virJSONValueObjectCreate(propsret,
> - "s:path", socketPath,
> + "s:path", disk->src->pr->path,
> NULL) < 0)
> goto cleanup;
>
> @@ -9713,14 +9707,12 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
> ret = 0;
> cleanup:
> VIR_FREE(alias);
> - VIR_FREE(socketPath);
> return ret;
> }
>
>
> static int
> -qemuBuildMasterPRCommandLine(virDomainObjPtr vm,
> - virCommandPtr cmd,
> +qemuBuildMasterPRCommandLine(virCommandPtr cmd,
> const virDomainDef *def)
> {
> size_t i;
> @@ -9740,7 +9732,7 @@ qemuBuildMasterPRCommandLine(virDomainObjPtr vm,
> managedAdded = true;
> }
>
> - if (qemuBuildPRManagerInfoProps(vm, disk, &props, &alias) < 0)
> + if (qemuBuildPRManagerInfoProps(disk, &props, &alias) < 0)
> goto cleanup;
>
> if (!props)
> @@ -9934,7 +9926,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
> if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0)
> goto error;
>
> - if (qemuBuildMasterPRCommandLine(vm, cmd, def) < 0)
> + if (qemuBuildMasterPRCommandLine(cmd, def) < 0)
Rather than @vm - what was really desired is/was @priv, which is already
passed for qemuBuildMasterKeyCommandLine and could be for this too...
Thus not requiring the hunks that change path in disk->src->pr->path.
It is a static path beyond the priv->libDir part.
> goto error;
>
> if (enableFips)
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index da1fe679fe..621592cd79 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -55,8 +55,7 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver,
> int **nicindexes);
>
> /* Generate the object properties for pr-manager */
> -int qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
> - const virDomainDiskDef *disk,
> +int qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
> virJSONValuePtr *propsret,
> char **alias);
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index eaa796260c..92217e66fe 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -11930,6 +11930,22 @@ qemuDomainPrepareDiskCachemode(virDomainDiskDefPtr disk)
> }
>
>
> +static int
> +qemuDomainPrepareStorageSourcePR(virStorageSourcePtr src,
> + qemuDomainObjPrivatePtr priv)
> +{
> + if (!src->pr)
> + return 0;
> +
> + if (virStoragePRDefIsManaged(src->pr)) {
> + if (!(src->pr->path = qemuDomainGetManagedPRSocketPath(priv)))
> + return -1;
> + }
While I understand the eventual goal - assigning a path here would seem
to be "contrary" to qemuDomainValidateStorageSource in the previous
patch. IOW: ->path should not be provided for managed reservations.
Doesn't that mean from this point forward we need to be careful to not
save the resulting path for the persistent XML? Or am I lost in the
weeds again?
It should never be saved in persistent XML since the parser+validator
will not allow parsing it. Since we don't modify the persistent XML in
this case it's not a problem.
The validation code is not called for the status XML so that should be
covered as well.
If this is to stay, then is this perhaps where :
<reservations managed='yes'>
<source type='unix' path='/somepath/ux.sck'
mode='client'/>
</reservations>
from patch 12 should be included for
tests/qemustatusxml2xmldata/modern-in.xml ?
Hmm, yeah we can add that too.