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

[Eric Blake]

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.

> +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.

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.

> +     * 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

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.

> +
> +    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.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110415/91e28068/attachment-0001.sig>