[libvirt] [PATCHv9 1/4] libvirt/qemu - persistent modification of devices.

[KAMEZAWA Hiroyuki]

On Fri, 15 Apr 2011 15:25:01 -0600
Eric Blake <eblake at redhat.com> wrote:

> On 04/12/2011 08:49 PM, KAMEZAWA Hiroyuki wrote:
> > 
> > Now, qemudDomainAttachDeviceFlags() and qemudDomainDetachDeviceFlags()
> > doesn't support VIR_DOMAIN_DEVICE_MODIFY_CONFIG. By this, virsh's
> > at(de)tatch-device --persistent cannot modify qemu config.
> > (Xen allows it.)
> > 
> > This patch is a base patch for adding support of devices in
> > step by step manner. Following patches will add some device
> > support.
> > 
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu at jp.fujitsu.com>
> > 
> > Changelog: v8->v9
> >   - removed unnecessary comments.
> 
> It's taken quite a few iterations, but I've finally scheduled time to
> start reviewing it.
> 

Thank you.

> > +static int qemuDomainModifyDevicePersistent(virDomainPtr dom,
> > +                                            const char *xml,
> > +                                            unsigned int attach,
> 
> You missed one.  You changed attach and detach, but not update.  This
> parameter can usefully forward to all three types of updates.
> 
> In fact, I think I'd like to go one further, and refactor things further
> before we start adding persistent devices.
> 

Hmm.

> It seems like there are two steps in all three categories of functions -
> obtain the locks, then do the work.  I like how you've factored things
> into one function that obtains the locks then forwards on to other
> functions that do the work.  Furthermore, I don't see any reason why we
> can't support LIVE|CONFIG at once, provided we know for which devices we
> can make a successful CONFIG change.
>


ok.

 
> > +     * When both of MODIFY_CONFIG and MODIFY_LIVE are passed at the same time,
> > +     * return error for now. We should support this later.
> > +     */
> > +    if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
> > +        qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > +                        _("cannot modify active domain and its definition "
> > +                          "at the same time."));
> > +        return -1;
> > +    }
> 
> You are right that it's not implemented yet, but no worse than what we
> already have, so it's not a regression.  But I think it is doable, by
> rewriting this into one giant function:
> 
> 1. obtain lock
> 2. if CURRENT, convert to LIVE or CONFIG
> 3. if LIVE but not active, error
> 4. if CONFIG but not persistent, error
> 5. if CONFIG call, make temporary vmdef and modify that, or quit if that
> device is not supported yet
> 6. if LIVE, make live modification; if that errors out, quit
> 7. if CONFIG, make temporary vmdef permanent (hopefully it succeeds,
> because we don't roll back the LIVE portion of LIVE|CONFIG)
> 8. clean up locks
> 

Sure, I'll include that logic.



> I'll propose a followup patch that tries to capture this idiom in code.
> 
> > +static int qemudDomainAttachDeviceFlags(virDomainPtr dom,
> > +                                        const char *xml,
> > +                                        unsigned int flags)
> > +{
> > +    int ret = -1;
> > +
> > +    virCheckFlags((VIR_DOMAIN_DEVICE_MODIFY_CONFIG |
> > +                   VIR_DOMAIN_DEVICE_MODIFY_LIVE), -1);
> 
> This causes a change in behavior.  Previously, we silently ignored
> VIR_DOMAIN_DEVICE_MODIFY_FORCE, now we error out.  But overall, that's a
> good change (FORCE only made sense for ModifyDevice, not Attach), and it
> goes to show that we don't use virCheckFlags on nearly enough APIs.
> 

will fix.


> > +
> > +    if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)
> > +        ret = qemuDomainModifyDevicePersistent(dom, xml, 1, flags);
> > +    else if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
> > +        ret = qemudDomainAttachDevice(dom, xml);
> 
> For example, here your logic is wrong.  You covered the CONFIG|LIVE case
> with qemuDomainModifyDevicePersistent (by erroring out), but you _don't_
> cover the CURRENT case (which should be either CONFIG or LIVE).  But to
> learn if the vm is active, you have to obtain the lock, and the way
> you've written this, you've already forwarded into one of the two
> routines.  Rather, all the public entries should forward into the one
> giant helper routine which grabs the locks, checks the flags, then calls
> into the right callbacks.
> 
> > @@ -4141,14 +4242,19 @@ cleanup:
> >  
> >  static int qemudDomainDetachDeviceFlags(virDomainPtr dom,
> >                                          const char *xml,
> > -                                        unsigned int flags) {
> > -    if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
> > -        qemuReportError(VIR_ERR_OPERATION_INVALID,
> > -                        "%s", _("cannot modify the persistent configuration of a domain"));
> > -        return -1;
> > -    }
> > +                                        unsigned int flags)
> > +{
> > +    int ret = -1;
> > +
> > +    virCheckFlags((VIR_DOMAIN_DEVICE_MODIFY_CONFIG |
> > +                   VIR_DOMAIN_DEVICE_MODIFY_LIVE), -1);
> 
> Another change in noisily rejecting FORCE, but I think it's good.
> 

I'll learn your patch and merge it to mine. It seems longer way than I thought.

Thanks,
-Kame