[libvirt] [PATCH 1/2] libvirt/qemu - support updating inactive domains (take 2)

[KAMEZAWA Hiroyuki]

On Thu, 24 Feb 2011 15:29:46 +0800
Hu Tao <hutao at 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