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

[John Ferlan]

On 04/16/2018 10:56 AM, Michal Privoznik wrote:
> 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.
> 
> 

True, especially with the amount/rate of change in the capabilities
area!  In some ways it's, point it out, let you know I ACKnowledge that
you'll have some merge work, but that work isn't something that'd be a
problem w/r/t the overall patch.

>>> 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.
> 

oh, right... "pr-manager-helper" is the object name, <sigh>

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

OK - I had scanned that review and had that comment in mind, but didn't
correlate the two when going through this (nor go back and check here
once I looked at hotplug later).

John