On 07/21/2016 07:43 AM, Bjoern Walk wrote:

John Ferlan <jferlan@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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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