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@xxxxxxxxxx>
>>> ---
>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
