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