[libvirt] [PATCH 4/4] qemu: add plumbing for persistent device changes

[Wen Congyang]

At 04/16/2011 11:28 PM, Eric Blake Write:
> There's still work to add persistent callback functions, and to
> make sure this all works, but this demonstrates how having a
> single function makes it easy to support flags for all three
> types of device modifications.
> 
> * src/qemu/qemu_driver.c (qemuDomainModifyDeviceFlags): Add
> parameter, and support VIR_DOMAIN_DEVICE_MODIFY_CURRENT.
> (qemuDomainAttachDeviceFlags, qemuDomainUpdateDeviceFlags)
> (qemuDomainDetachDeviceFlags): Update callers.
> ---
> 
> After this point, we can use Kame's patch 1/4 to add in the
> persistent function callbacks (with slight tweaks to their
> signatures), and 2/4 to make it easier to to temporary
> modifications to a config:
> 
> if (flags & CONFIG) {
>    create temporary def
>    call persistent callback
> }
> if (flags & LIVE) {
>    call live callback
> }
> if (no errors)
>    commit temporary def and live state changes, as needed
> 
> But with the benefit that CURRENT support is now provided.
> 
>  src/qemu/qemu_driver.c |   47 +++++++++++++++++++++++++++++++++++------------
>  1 files changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 4f0a057..8c978be 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4031,7 +4031,8 @@ cleanup:
>  static int
>  qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
>                              unsigned int flags,
> -                            qemuDomainModifyDeviceCallback cb)
> +                            qemuDomainModifyDeviceCallback live,
> +                            qemuDomainModifyDeviceCallback persistent)
>  {
>      struct qemud_driver *driver = dom->conn->privateData;
>      virDomainObjPtr vm;
> @@ -4039,12 +4040,7 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
>      virBitmapPtr qemuCaps = NULL;
>      int ret = -1;
>      bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0;
> -
> -    if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
> -        qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> -                        _("cannot modify domain persistent configuration"));
> -        return -1;
> -    }
> +    bool active;
> 
>      qemuDriverLock(driver);
>      vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> @@ -4059,12 +4055,32 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
>      if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
>          goto cleanup;
> 
> -    if (!virDomainObjIsActive(vm)) {
> +    active = virDomainObjIsActive(vm);
> +
> +    if ((flags & (VIR_DOMAIN_DEVICE_MODIFY_LIVE
> +                  | VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) == 0)
> +        flags |= (active ? VIR_DOMAIN_DEVICE_MODIFY_LIVE
> +                  : VIR_DOMAIN_DEVICE_MODIFY_CONFIG);
> +
> +    if ((flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) && !active) {
>          qemuReportError(VIR_ERR_OPERATION_INVALID,
>                          "%s", _("cannot modify device on inactive domain"));
>          goto endjob;
>      }
> 
> +    if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) {
> +        qemuReportError(VIR_ERR_OPERATION_INVALID,
> +                        "%s", _("cannot modify device on transient domain"));
> +        goto endjob;
> +    }
> +
> +    /* XXX add persistent support */
> +    if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {

kamezawa-san is writing patch to support persistent device modification(attach/detach)
and Hu Tao is writing patch to support persistent device modification(update).

We can detect whether persistent device modification is supported like this:
if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)  && !persistent) {

So the caller can pass NULL when persistent device modification is not supported safely.

> +        qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                        _("cannot modify domain persistent configuration"));
> +        return -1;

We should goto endjob to do some cleanup.

> +    }
> +
>      dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
>                                    VIR_DOMAIN_XML_INACTIVE);
>      if (dev == NULL)
> @@ -4075,7 +4091,11 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
>                                     &qemuCaps) < 0)
>          goto endjob;
> 
> -    ret = (cb)(dom, driver, vm, dev, qemuCaps, force);
> +    ret = 0;
> +    if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
> +        ret = (live)(dom, driver, vm, dev, qemuCaps, force);
> +    if (ret == 0 && (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG))
> +        ret = (persistent)(dom, driver, vm, dev, qemuCaps, force);
> 
>      /* update domain status forcibly because the domain status may be changed
>       * even if we attach the device failed. For example, a new controller may
> @@ -4105,7 +4125,8 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
>      virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
>                    VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
>      return qemuDomainModifyDeviceFlags(dom, xml, flags,
> -                                       qemuDomainAttachDeviceLive);
> +                                       qemuDomainAttachDeviceLive,
> +                                       NULL);
>  }
> 
>  static int
> @@ -4124,7 +4145,8 @@ qemuDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml,
>                    VIR_DOMAIN_DEVICE_MODIFY_CONFIG |
>                    VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
>      return qemuDomainModifyDeviceFlags(dom, xml, flags,
> -                                       qemuDomainUpdateDeviceLive);
> +                                       qemuDomainUpdateDeviceLive,
> +                                       NULL);
>  }
> 
> 
> @@ -4135,7 +4157,8 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
>      virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
>                    VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
>      return qemuDomainModifyDeviceFlags(dom, xml, flags,
> -                                       qemuDomainDetachDeviceLive);
> +                                       qemuDomainDetachDeviceLive,
> +                                       NULL);
>  }
> 
>  static int

The other modification and the other 3 patches look good to me.