[libvirt] [PATCH v4 08/14] qemu: Generate cmd line at startup

[Michal Privoznik]

On 04/14/2018 03:04 PM, John Ferlan wrote:
> 
> 
> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>> This is the easier part. All we need to do here is put -object
>> pr-manager-helper,id=$alias,path=$socketPath and then just
>> reference the object in -drive file.pr-manager=$alias.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/qemu/qemu_command.c                            | 94 ++++++++++++++++++++++
>>  src/qemu/qemu_command.h                            |  3 +
>>  .../disk-virtio-scsi-reservations.args             | 35 ++++++++
>>  tests/qemuxml2argvtest.c                           |  4 +
>>  4 files changed, 136 insertions(+)
>>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
>>
> 
> This patch fails qemuxml2argv for me because of commits 'cc32731a' and
> 'a32539de'...
> 

Oh yeah. There's always chance that between me posting patches and
review somebody will push something that invalidates my patches.


>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 514c3ab2ef..81f6025207 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1514,6 +1514,20 @@ qemuDiskSourceGetProps(virStorageSourcePtr src)
>>  }
>>  
>>  
>> +static void
>> +qemuBuildDriveSourcePR(virBufferPtr buf,
>> +                       virStorageSourcePtr src)
>> +{
>> +    qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
>> +    qemuDomainDiskPRDPtr prd = srcPriv->prd;
>> +
>> +    if (!prd || !prd->alias)
>> +        return;
>> +
>> +    virBufferAsprintf(buf, ",file.pr-manager=%s", prd->alias);
>> +}
>> +
>> +
>>  static int
>>  qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>>                          virQEMUCapsPtr qemuCaps,
>> @@ -1591,6 +1605,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>>  
>>          if (disk->src->debug)
>>              virBufferAsprintf(buf, ",file.debug=%d", disk->src->debugLevel);
>> +
>> +        qemuBuildDriveSourcePR(buf, disk->src);
>>      } else {
>>          if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops)))
>>              goto cleanup;
>> @@ -9872,6 +9888,81 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
>>  }
>>  
>>  
>> +/**
>> + * qemuBuildPRManagerInfoProps:
>> + * @prd: disk PR runtime info
>> + * @propsret: JSON properties to return
>> + *
>> + * Build the JSON properties for the pr-manager object.
>> + *
>> + * Returns: 0 on success (@propsret is NULL if no properties are needed),
>> + *         -1 on failure (with error message set).
>> + */
>> +int
>> +qemuBuildPRManagerInfoProps(qemuDomainDiskPRDPtr prd,
>> +                            virJSONValuePtr *propsret)
>> +{
>> +    *propsret = NULL;
>> +
>> +    if (!prd || !prd->alias)
>> +        return 0;
>> +
>> +    if (virJSONValueObjectCreate(propsret,
>> +                                 "s:path", prd->path,
>> +                                 NULL) < 0)
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> +
>> +
>> +static int
>> +qemuBuildMasterPRCommandLine(virCommandPtr cmd,
>> +                             const virDomainDef *def)
>> +{
>> +    size_t i;
>> +    bool managedAdded = false;
>> +    virJSONValuePtr props = NULL;
>> +    char *tmp = NULL;
>> +    int ret = -1;
>> +
>> +    for (i = 0; i < def->ndisks; i++) {
>> +        const virDomainDiskDef *disk = def->disks[i];
>> +        qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
>> +        qemuDomainDiskPRDPtr prd = srcPriv->prd;
>> +
>> +
>> +        if (virStoragePRDefIsManaged(disk->src->pr)) {
>> +            if (managedAdded)
>> +                continue;
>> +
>> +            managedAdded = true;
> 
> As soon as we find one that needs managing we should break from the loop
> and then only add pr-manager-helper if we have one to manage. No sense
> going through the rest of the list of disks.  Move @disk into the top
> level and then use it to decide whether or not to add the object.  Then
> in some way signify that the object was added so that future hotplugs
> wouldn't need to somehow guess - a simple check that it was added
> already would seem to suffice.

Not true. We need to add manager objects even for cases where libvirt is
not managing the helper process. For instance, for the following XML:

  <disk type='block' device='lun'>
    <source dev='/dev/HostVG/QEMUGuest2'>
      <reservations enabled='yes' managed='no'>
        <source type='unix' path='/path/to/qemu-pr-helper.sock' mode='client'/>
      </reservations>
    </source>
  </disk>

the following cmd line needs to be generated:

  -object pr-manager-helper,id=pr-helper-scsi0-0-0-1,\
          path=/path/to/qemu-pr-helper.sock \

otherwise qemu would now know where to connect. However, if libvirt is
managing the pr-helper process, all the managed disks share the same
process => object on the command line => it must be added exactly once.

> 
>> +        }
> 
> The next hunk would seem to be useful for the hotplug path, no?
> Including perhaps setting some sort of qemu_domain level boolean that
> the object was added already so that subsequent or consecutive hotplugs
> wouldn't try to add it again.

It's reused there yes. As Peter suggested in one of earlier reviews
instead of having two functions (one for generating cmd line and the
other for JSON) I can write just one and use
virQEMUBuildObjectCommandlineFromJSON() to translate JSON into cmd line.

Michal