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

[Michal Privoznik]

On 04/29/2018 02:16 PM, John Ferlan wrote:
> 
> 
> 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.

I rather not. Everything else uses a function to get/set aliases. In
order to stay consistent I'd rather have this as a function.

> 
> 
> Ironically later on, we have:
> 
> #define QEMU_PR_HELPER "/usr/bin/qemu-pr-helper"

Do we? Not in the code, but in config.h produced by configure. Well,
that's standardized way of defining values in configure. I'm not sure
that logic applies to the rest of our code.

> 
> and use
> 
>         VIR_STRDUP(cfg->prHelperName, QEMU_PR_HELPER) < 0)
> 
> So why not use the same construct for this alias?

Because QEMU_PR_HELPER is detected at compile time (for the default, can
be overridden in qemu.conf later). There is no other way to do it. We
can't have configure generating a function like this:

  qemuGetPrHelperPath() {
    return "/usr/bin/qemu-pr-helper";
  }

But we can have our own code do that, esp. if other functions from the
family (qemu alias handling functions that is) are functions indeed
instead of some #define-s.

> 
>> +}
>> +
>> +
>> +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/
> 

Oh, right.

Michal