[libvirt] [PATCH 3/8] qemu: hotplug: Use new source when preparing/translating for media change

[John Ferlan]

On 10/2/18 8:25 AM, Peter Krempa wrote:
> On Tue, Oct 02, 2018 at 08:15:07 -0400, John Ferlan wrote:
>>
>>
>> On 9/27/18 11:09 AM, Peter Krempa wrote:
>>> Some internal helpers use only a virDomainDiskDef to do their job. If we
>>> want to change source for the disk we need to update it to the new
>>> source in the disk definition for those to work correctly.
>>>
>>> Signed-off-by: Peter Krempa <pkrempa at redhat.com>
>>> ---
>>>  src/qemu/qemu_hotplug.c | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>> index 86afda636e..3043b3257b 100644
>>> --- a/src/qemu/qemu_hotplug.c
>>> +++ b/src/qemu/qemu_hotplug.c
>>> @@ -730,9 +730,12 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>>>                                 bool force)
>>>  {
>>>      qemuDomainObjPrivatePtr priv = vm->privateData;
>>> +    virStorageSourcePtr oldsrc = disk->src;
>>>      int ret = -1;
>>>      int rc;
>>>
>>> +    disk->src = newsrc;
>>> +
>>
>> Doesn't seem like this would be right especially considering what each
>> of the following calls passing both @disk and @newsrc do.
>>
>>>      if (qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, false) < 0)
>>>          goto cleanup;
>>
>> Passing both @disk and @newsrc here after assigning newsrc into
>> disk->src would seem to do nothing since it temporarily swaps them for
>> processing, then returns with them swapped back.
> 
> Umm, well it's kind of pointless to pass in newsrc, but it's the exactly
> expected behaviour. We want to prepare 'newsrc' but half of the
> functions take disk->src directly.
> 

Yeah, true - if both this and the one below pass NULL, then nothing in
Prepare needs @newsrc... Then it just becomes a process of passing the
right one at the right time.

>>
>> Shortly after this passing both @disk and @newsrc into either
>> qemuDomainChangeMediaBlockdev or qemuDomainChangeMediaLegacy would seem
>> to be incorrect especially since each deals with what's in the drive
>> first which would now be incorrect, wouldn't it?
> 
> Hmm, yeah, the blockdev version actually does care what the old source
> was, so that would not work.
> 
> The non-blockdev version does not care so it does'nt matter much.

Seems to me qemuDomainChangeMediaLegacy manipulates @disk (oldsrc) and
needs diskPriv just like qemuDomainChangeMediaBlockdev.

John

> 
> I'll have to change qemuDomainChangeMediaBlockdev to take 'oldsrc'
> instead of 'newsrc'.
> 
>>
>>>
>>> @@ -744,7 +747,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>>>      else
>>>          rc = qemuDomainChangeMediaLegacy(driver, vm, disk, newsrc, force);
>>>
>>> -    virDomainAuditDisk(vm, disk->src, newsrc, "update", rc >= 0);
>>> +    virDomainAuditDisk(vm, oldsrc, newsrc, "update", rc >= 0);
>>>
>>>      if (rc < 0) {
>>>          ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, true));
>>
>> This portion would seemingly be problematic too.
> 
> As explained above, the only thing that happens is that disk->src is
> replaced and reverted again, thus 3 extra assignments.
>