Re: [libvirt] ambiguous ret of qemuDomainDetachVirtioDiskDevice

On 2015/7/30 17:41, zhang bo wrote: > On 2015/7/28 16:33, Ján Tomko wrote: > >> On Tue, Jul 28, 2015 at 04:25:13PM +0800, zhang bo wrote: >>> static int >>> qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, >>> virDomainObjPtr vm, >>> virDomainDiskDefPtr detach) >>> { >>> ....... >>> >>> rc = qemuDomainWaitForDeviceRemoval(vm); >>> if (rc == 0 || rc == 1) >>> ret = qemuDomainRemoveDiskDevice(driver, vm, detach); >>> else >>> ret = 0; /*the return value of 2 is dismissed here, which refers to ETIMEOUT.*/ >>> ........ >>> } >>> >>> ------------------------------------ >>> >>> If it timeouts when qemu tries to del the device, the return value would be modified from 2 to 0 in >>> function qemuDomainDetachVirtioDiskDevice(), which means that, the users would be misleaded that >>> the device has been deleted, however, the device maybe probably failed to be detached after timeout and >>> still in use. >>> >> >> This is intentional and documented: >> http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainDetachDevice... >> >> Unplugging a disk requires guest cooperation, so the best we can do is >> ask qemu to unplug it and wait for a while. >> >>> That is to say, the function qemuDomainDetachVirtioDiskDevice()'s return value is ambiguous when it's 0, >>> maybe successful, or timeout. Will it be better to pass ETIMEOUT to user? or any other advises? for example, >>> let users themselves dumpxml the guest to check whether the device has been actually detached or not? >> >> Either dump the XML, or wait for the VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED >> event, as the API documentation suggests. >> >> Jan > > > It seems to have fixed the problem by dumping the XML or wait for the DEVICE_REMOVED event. However, it seems to > make nova or other apps to do more checking work, they need to dump XML or wait the event even if the device has > already been successfully removed. which is unnecessary. > > I think, it's better to return ETIMEOUT and let nova to dumpxml or wait the event at this situation, rather than always > doing that job. >

> It maybe a better design, what's your opinion? > After thinking twice, it's an async job, thus returning 0 is acceptable, right?