Re: [libvirt] [PATCH] Return error when updating cdrom device

On 05/19/14 16:07, Pavel Hrdina wrote: > The commit 84c59ffa improved the way we change ejectable media. > If for any reason the first "eject" didn't open the tray we > should return with error. > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > src/qemu/qemu_hotplug.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 125a2db..47ec779 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -125,6 +125,7 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, > /* If ret == -1, EjectMedia already set an error message */ > virReportError(VIR_ERR_OPERATION_FAILED, "%s", > _("Unable to eject media")); > + ret = -1; Although this is technically correct, the control flow in this function seems a little bit weird. The eject media error should be checked before the polling loop and removed from here. Then also the ret=0 asignment a few lines below isn't useful. > } > goto audit; > } > I'd go with the following patch: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 125a2db..76e289b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -106,6 +106,9 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force); qemuDomainObjExitMonitor(driver, vm); + if (ret < 0) + goto audit; + virObjectRef(vm); /* we don't want to report errors from media tray_open polling */ while (retries) { @@ -121,14 +124,11 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virObjectUnref(vm); if (retries <= 0) { - if (ret == 0) { - /* If ret == -1, EjectMedia already set an error message */ - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Unable to eject media")); - } + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Unable to eject media")); + ret = -1; goto audit; } - ret = 0; src = virDomainDiskGetSource(disk); if (src) { ACK if you go with the above. Peter