[libvirt] [PATCH v5 04/11] qemu: Generate pr cmd line at startup

[John Ferlan]

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 at redhat.com>
> ---
>  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 at redhat.com>

a less reluctant one mean change to use the # define construct - I've
even attached a diff that worked for me.

John

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Use-constant-for-managed-pr-helper-alias.patch
Type: text/x-patch
Size: 3720 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180429/4a8c91d5/attachment-0001.bin>