On Fri, 15 Apr 2011 15:25:01 -0600
Eric Blake <eblake@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> >
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list