Re: [libvirt] [PATCH 3/8] qemu: hotplug: Use new source when preparing/translating for media change
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
>>> ---
>>> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/8] qemu: hotplug: Use new source when preparing/translating for media change
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
> > ---
> > 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.
>
> 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.
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/8] qemu: hotplug: Use new source when preparing/translating for media change
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
> ---
> 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.
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?
>
> @@ -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.
Hard to review the rest...
John
> @@ -755,7 +758,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name));
> ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true));
>
> -virStorageSourceFree(disk->src);
> +virStorageSourceFree(oldsrc);
> +oldsrc = NULL;
> VIR_STEAL_PTR(disk->src, newsrc);
>
> ignore_value(qemuHotplugRemoveManagedPR(driver, vm,
> QEMU_ASYNC_JOB_NONE));
> @@ -763,6 +767,9 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> ret = 0;
>
> cleanup:
> +if (oldsrc)
> +disk->src = oldsrc;
> +
> return ret;
> }
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/8] qemu: hotplug: Use new source when preparing/translating for media change
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
---
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;
+
if (qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, false) < 0)
goto cleanup;
@@ -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));
@@ -755,7 +758,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name));
ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true));
-virStorageSourceFree(disk->src);
+virStorageSourceFree(oldsrc);
+oldsrc = NULL;
VIR_STEAL_PTR(disk->src, newsrc);
ignore_value(qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE));
@@ -763,6 +767,9 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
ret = 0;
cleanup:
+if (oldsrc)
+disk->src = oldsrc;
+
return ret;
}
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
