On Thu, 24 Feb 2011 15:29:46 +0800 Hu Tao <hutao@cn.fujitsu.com> wrote:
+ + driver = dom->conn->privateData; + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + qemuReportError(VIR_ERR_NO_DOMAIN, _("cannot find domain '%s'"), + dom->name); + goto unlock_out; + } + + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) { + /* + * For now, just allow updating inactive domains. Further development + * will allow updating both active domain and its config file at + * the same time. + */ + qemuReportError(VIR_ERR_INVALID_ARG, + _("cannot update alive domain : %s"), __FUNCTION__);
I think we can silently fail here. Currently this function (qemuDomainModifyDevicePersistent) is called only for modifying inactive domains.
It seems...."virDomainObjIsActive(vm) check" is lacked..
Yes. What about checking for flag VIR_DOMAIN_DEVICE_MODIFY_LIVE? This flag is set if domain is active, for example, see cmdAttachDisk().
checking virDomainObjIsActive(vm) under lock is required. As you pointed out, if --persistent is specified in virsh attach-disk, == if (vshCommandOptBool(cmd, "persistent")) { flags = VIR_DOMAIN_DEVICE_MODIFY_CONFIG; if (virDomainIsActive(dom) == 1) flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; ret = virDomainAttachDeviceFlags(dom, buffer, flags); } else { ret = virDomainAttachDevice(dom, buffer); } == Both of MODIFY_CONFIG, MODIFY_LIVE flags are always set if domain is alive. * Attach a virtual device to a domain, using the flags parameter * to control how the device is attached. VIR_DOMAIN_DEVICE_MODIFY_CURRENT * specifies that the device allocation is made based on current domain * state. VIR_DOMAIN_DEVICE_MODIFY_LIVE specifies that the device shall be * allocated to the active domain instance only and is not added to the * persisted domain configuration. VIR_DOMAIN_DEVICE_MODIFY_CONFIG * specifies that the device shall be allocated to the persisted domain * configuration only. Note that the target hypervisor must return an * error if unable to satisfy flags. E.g. the hypervisor driver will * return failure if LIVE is specified but it only supports modifying the * persisted device allocation.
From above, if VIR_DOMAIN_DEVICE_MODIFY_LIVE is set....this function should fail? Ok. But no explanation of API when both of MODIFY_LIVE and MODIFY_CONFIG are set at the same time.
I'll add a test of VIR_DOMAIN_DEVICE_MODIFY_LIVE for now. But I'll remove it later. What we want to know here is the caller wants persistent change or not. I think I'll update this function to allow modification of active domain in future. Thanks, -Kame