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@xxxxxxxxxx>
> > ---
> > 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.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
