[libvirt] [PATCH v2 06/12] qemu: Generate cmd line at startup

[John Ferlan]

On 03/06/2018 12:31 PM, Michal Privoznik wrote:
> On 03/02/2018 02:22 PM, John Ferlan wrote:
>>
>>
>> On 02/21/2018 01:11 PM, Michal Privoznik wrote:
>>> This is the easier part. All we need to do here is put -object
>>> pr-manger-helper,id=$alias,path=$socketPath and then just
>>> reference the object in -drive file.pr-manger=$alias.
>>
>> s/manger/manager/
>>
>> My fingers usually the same way though as manger
>>
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>> ---
>>>  src/qemu/qemu_command.c                            | 40 ++++++++++++++++++++++
>>>  .../disk-virtio-scsi-reservations-not-managed.args | 28 +++++++++++++++
>>>  .../disk-virtio-scsi-reservations.args             | 29 ++++++++++++++++
>>>  tests/qemuxml2argvtest.c                           |  8 +++++
>>>  4 files changed, 105 insertions(+)
>>>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.args
>>>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
>>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index fa0aa5d5c..069d60d35 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,
>>> @@ -1590,6 +1604,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;
>>> @@ -9789,6 +9805,28 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
>>>  }
>>>  
>>>  
>>> +static void
>>> +qemuBuildMasterPRCommandLine(virCommandPtr cmd,
>>> +                             const virDomainDef *def)
>>> +{
>>> +    size_t i;
>>> +
>>> +    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;
>>> +        virBuffer buf = VIR_BUFFER_INITIALIZER;
>>> +
>>> +        if (!prd || !prd->alias)
>>> +            continue;
>>> +
>>> +        virBufferAsprintf(&buf, "pr-manager-helper,id=%s,path=%s", prd->alias, prd->path);
>>> +        virCommandAddArg(cmd, "-object");
>>> +        virCommandAddArgBuffer(cmd, &buf);
>>
>> What happens when there's more than one disk using the managed mode
>> where you have a "static" alias and path, wouldn't there be multiple
>> lines with:
>>
>> -object
>> pr-manager-helper,id=pr-helper0,path=/tmp/lib/domain--1-QEMUGuest1/pr-helper0.sock
> 
> Ah sure.
> 

So having multiple "id=pr-helper0" is OK by QEMU?  OK, then.

John

>>
>> ?  And how is QEMU going to react to that?
>>
>> IOW: Shouldn't this code know it's already created an object for that
>> case and not generate another one?
>>
>> The other one :
>>
>> -object pr-manager-helper,id=pr-helper-sdb,path=/path/to/qemu-pr-helper.sock
>>
>> I get, but I'm still not thrilled with "sdb" as opposed to the
>> disk->info.alias of "scsi0-0-0-0" more commonly used. IIRC, there's no
>> guarantee that what libvirt calls "sdb" ends up being "sdb" on the
>> guest. My vague recollection of the algorithm that "automagically"
>> generates the address based on sda, sdb, sdc, etc. So "sdb" IIRC would
>> related to an address that would create an alias using "0-0-1"; whereas,
>> "sda" would create that "0-0-0" value.
> 
> Yes. I've changed it in the other patch so now 'pr-helper-scsi0-0-0-0'
> is generated.
> 
> Michal
>