5:30 a.m.
Since return code is checked globally at the end of the function, let's
make sure that we set it correctly at any point.
This fixes a regression introduced in commit 0aa19f35 where the first
command to eject changeable media would fail unconditionally.
Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/qemu/qemu_hotplug.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c322543..789f18c 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -240,7 +240,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
/* If the tray is present and tray change event is supported wait for it to open. */
if (diskPriv->tray &&
virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_TRAY_MOVED)) {
- if (qemuHotplugWaitForTrayEject(driver, vm, disk, driveAlias, force) < 0)
+ rc = qemuHotplugWaitForTrayEject(driver, vm, disk, driveAlias, force);
+ if (rc < 0)
goto error;
} else {
/* otherwise report possible errors from the attempt to eject the media*/
--
2.6.6
1:43 a.m.
Bjoern Walk <bwalk(a)linux.vnet.ibm.com> [2016-07-08, 12:34PM +0200]:
Since return code is checked globally at the end of the function,
let's
make sure that we set it correctly at any point.
This fixes a regression introduced in commit 0aa19f35 where the first
command to eject changeable media would fail unconditionally.
Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/qemu/qemu_hotplug.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c322543..789f18c 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -240,7 +240,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
/* If the tray is present and tray change event is supported wait for it to open. */
if (diskPriv->tray &&
virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_TRAY_MOVED)) {
- if (qemuHotplugWaitForTrayEject(driver, vm, disk, driveAlias, force) < 0)
+ rc = qemuHotplugWaitForTrayEject(driver, vm, disk, driveAlias, force);
+ if (rc < 0)
goto error;
} else {
/* otherwise report possible errors from the attempt to eject the media*/
--
2.6.6
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Ping.
5:23 a.m.
On 07/08/2016 06:30 AM, Bjoern Walk wrote:
Since return code is checked globally at the end of the function,
let's
make sure that we set it correctly at any point.
This fixes a regression introduced in commit 0aa19f35 where the first
command to eject changeable media would fail unconditionally.
Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/qemu/qemu_hotplug.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c322543..789f18c 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -240,7 +240,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
/* If the tray is present and tray change event is supported wait for it to open.
*/
if (diskPriv->tray &&
virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_TRAY_MOVED)) {
- if (qemuHotplugWaitForTrayEject(driver, vm, disk, driveAlias, force) < 0)
+ rc = qemuHotplugWaitForTrayEject(driver, vm, disk, driveAlias, force);
+ if (rc < 0)
goto error;
But rc isn't checked in the error and cleanup labels in
qemuDomainChangeEjectableMedia, so not sure what the purpose of setting
rc is. What am I missing?
The existing "else" in the code is for (diskPriv->tray &&
virQEMU...)
condition as I see it.
If perhaps you had just set rc, not done the goto, and remove the else
condition, e.g.:
if (diskPriv... && virQEMUCaps...)
rc = qemuHotplugWait...
if (rc < 0)
goto error;
that's just doing the same thing in a different way.
John
} else {
/* otherwise report possible errors from the attempt to eject the media*/
6:43 a.m.
John Ferlan <jferlan(a)redhat.com> [2016-07-21, 12:23PM +0200]:
On 07/08/2016 06:30 AM, Bjoern Walk wrote:
> Since return code is checked globally at the end of the function, let's
> make sure that we set it correctly at any point.
>
> This fixes a regression introduced in commit 0aa19f35 where the first
> command to eject changeable media would fail unconditionally.
>
> Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> ---
> src/qemu/qemu_hotplug.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index c322543..789f18c 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -240,7 +240,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> /* If the tray is present and tray change event is supported wait for it to
open. */
> if (diskPriv->tray &&
> virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_TRAY_MOVED)) {
> - if (qemuHotplugWaitForTrayEject(driver, vm, disk, driveAlias, force) <
0)
> + rc = qemuHotplugWaitForTrayEject(driver, vm, disk, driveAlias, force);
> + if (rc < 0)
> goto error;
But rc isn't checked in the error and cleanup labels in
qemuDomainChangeEjectableMedia, so not sure what the purpose of setting
rc is. What am I missing?
It takes two issues of qemuMonitorEjectMedia in order to successfully
eject the media as far as I understood because the first only opens the
tray only. Hence
235 qemuDomainObjEnterMonitor(driver, vm);
236 rc = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
rc == -1 for the first call.
237 if (qemuDomainObjExitMonitor(driver, vm) < 0)
238 goto cleanup;
239
240 /* If the tray is present and tray change event is supported wait for it to open.
*/
241 if (diskPriv->tray &&
242 virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_TRAY_MOVED)) {
243 if (qemuHotplugWaitForTrayEject(driver, vm, disk, driveAlias, force) < 0)
This is the second call to qemuMonitorEjectMedia after waiting an
appropriate time to process the tray open command above. This call
succeeds and returns 0 but the return code rc is _not_ set, so rc is
still -1.
244 goto error;
245 } else {
246 /* otherwise report possible errors from the attempt to eject the media*/
247 if (rc < 0)
248 goto error;
249 }
250
251 if (!virStorageSourceIsEmpty(newsrc)) {
This doesn't get called for just an eject media command...
[...]
264 rc = qemuMonitorChangeMedia(priv->mon,
... but it _would_ actually set rc correctly.
265 driveAlias,
266 sourcestr,
267 format);
268 if (qemuDomainObjExitMonitor(driver, vm) < 0)
269 goto cleanup;
270 }
271
272 virDomainAuditDisk(vm, disk->src, newsrc, "update", rc >= 0);
273
274 if (rc < 0)
275 goto error;
Since rc is still -1 because it did not get set properly on each branch,
the function errors out despite actually succeeding. This is the actual
behaviour of this function prior to the refactoring, so I guess it's
just a simple oversight. Unfortunately, the test suite did not catch
this because of a missing mock in qemuhotplugtest.
The existing "else" in the code is for (diskPriv->tray
&& virQEMU...)
condition as I see it.
If perhaps you had just set rc, not done the goto, and remove the else
condition, e.g.:
if (diskPriv... && virQEMUCaps...)
rc = qemuHotplugWait...
if (rc < 0)
goto error;
that's just doing the same thing in a different way.
I don't understand. My patch restores the exact functional behaviour of
this function prior to the refactor. I do see a spurious error message
when issuing the first change-media --eject command. This is gone with
my patch applied. If the function is otherwise misbehaving I can not
tell.
John
> } else {
> /* otherwise report possible errors from the attempt to eject the media*/
>
Best,
Bjoern
5:55 a.m.
On 07/21/2016 07:43 AM, Bjoern Walk wrote:
John Ferlan <jferlan(a)redhat.com> [2016-07-21, 12:23PM +0200]:
>
>
> On 07/08/2016 06:30 AM, Bjoern Walk wrote:
>> Since return code is checked globally at the end of the function, let's
>> make sure that we set it correctly at any point.
>>
>> This fixes a regression introduced in commit 0aa19f35 where the first
>> command to eject changeable media would fail unconditionally.
>>
>> Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
>> ---
>> src/qemu/qemu_hotplug.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index c322543..789f18c 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -240,7 +240,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr
>> driver,
>> /* If the tray is present and tray change event is supported
>> wait for it to open. */
>> if (diskPriv->tray &&
>> virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_TRAY_MOVED)) {
>> - if (qemuHotplugWaitForTrayEject(driver, vm, disk,
>> driveAlias, force) < 0)
>> + rc = qemuHotplugWaitForTrayEject(driver, vm, disk,
>> driveAlias, force);
>> + if (rc < 0)
>> goto error;
>
> But rc isn't checked in the error and cleanup labels in
> qemuDomainChangeEjectableMedia, so not sure what the purpose of setting
> rc is. What am I missing?
>
It takes two issues of qemuMonitorEjectMedia in order to successfully
eject the media as far as I understood because the first only opens the
tray only. Hence
235 qemuDomainObjEnterMonitor(driver, vm);
236 rc = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
rc == -1 for the first call.
237 if (qemuDomainObjExitMonitor(driver, vm) < 0)
238 goto cleanup;
239
240 /* If the tray is present and tray change event is supported
wait for it to open. */
241 if (diskPriv->tray &&
242 virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_TRAY_MOVED)) {
243 if (qemuHotplugWaitForTrayEject(driver, vm, disk,
driveAlias, force) < 0)
This is the second call to qemuMonitorEjectMedia after waiting an
appropriate time to process the tray open command above. This call
succeeds and returns 0 but the return code rc is _not_ set, so rc is
still -1.
Ahh - the success case. I was reading as the failure and goto case
probably because of the commit message.
So ACK to the patch and I just pushed
John
244 goto error;
245 } else {
246 /* otherwise report possible errors from the attempt to
eject the media*/
247 if (rc < 0)
248 goto error;
249 }
250
251 if (!virStorageSourceIsEmpty(newsrc)) {
This doesn't get called for just an eject media command...
[...]
264 rc = qemuMonitorChangeMedia(priv->mon,
... but it _would_ actually set rc correctly.
265 driveAlias,
266 sourcestr,
267 format);
268 if (qemuDomainObjExitMonitor(driver, vm) < 0)
269 goto cleanup;
270 }
271
272 virDomainAuditDisk(vm, disk->src, newsrc, "update", rc >= 0);
273
274 if (rc < 0)
275 goto error;
Since rc is still -1 because it did not get set properly on each branch,
the function errors out despite actually succeeding. This is the actual
behaviour of this function prior to the refactoring, so I guess it's
just a simple oversight. Unfortunately, the test suite did not catch
this because of a missing mock in qemuhotplugtest.
> The existing "else" in the code is for (diskPriv->tray &&
virQEMU...)
> condition as I see it.
>
> If perhaps you had just set rc, not done the goto, and remove the else
> condition, e.g.:
>
> if (diskPriv... && virQEMUCaps...)
> rc = qemuHotplugWait...
>
> if (rc < 0)
> goto error;
>
> that's just doing the same thing in a different way.
>
I don't understand. My patch restores the exact functional behaviour of
this function prior to the refactor. I do see a spurious error message
when issuing the first change-media --eject command. This is gone with
my patch applied. If the function is otherwise misbehaving I can not
tell.
> John
>> } else {
>> /* otherwise report possible errors from the attempt to
>> eject the media*/
>>
>
Best,
Bjoern
6:35 a.m.
John Ferlan <jferlan(a)redhat.com> [2016-07-22, 12:58PM +0200]:
[...]
Ahh - the success case. I was reading as the failure and goto case
probably because of the commit message.
Yeah, I probably should have explained this explicitly in the commit
message, sorry about that.
So ACK to the patch and I just pushed
Thanks a lot.
3035
days inactive
3049
days old
5 comments
2 participants
participants (2)
-
Bjoern Walk -
John Ferlan