[libvirt] [PATCH] Provide a helper method virDomainLiveHelperMethod

[Eric Blake]

On 12/07/2011 04:04 AM, Lei Li wrote:
> This chunk of code below repeated in several functions, factor it into
> a helper method virDomainLiveHelperMethod to eliminate duplicated code
> based on Eric and Adam's suggestion. I have tested it for all the
> relevant APIs changed.
> 
> 
> Signed-off-by: Lei Li <lilei at linux.vnet.ibm.com>
> ---
>  src/conf/domain_conf.c   |   47 ++++++++
>  src/conf/domain_conf.h   |    7 +
>  src/libvirt_private.syms |    1 +
>  src/qemu/qemu_driver.c   |  288 ++++------------------------------------------
>  4 files changed, 77 insertions(+), 266 deletions(-)

Nice size reduction!  Can any of the other drivers use it as well (maybe
libxl, lxc, test, uml)?

> +++ b/src/conf/domain_conf.c
> @@ -1670,6 +1670,53 @@ virDomainObjGetPersistentDef(virCapsPtr caps,
>  }
>  
>  /*
> + * Helper method for --current --live --config option, and check with
> + * whether domain is active or can get persistent domain configuration.
> + *
> + * Return 0 if success, also change the flags and get the persistent
> + * domain configuration if needed. Return -1 on error.
> + */
> +int
> +virDomainLiveConfigHelperMethod(virCapsPtr caps,
> +                                virDomainObjPtr dom,
> +                                unsigned int *flags,
> +                                virDomainDefPtr *persistentDef)

Looks like a reasonable name and location for sharing purposes.  Do we
also want to pass isactive back to the caller?

> +{
> +    bool isActive;
> +    int ret = 0;

Easier to start with ret = -1, then clear it to 0 on success.

> +
> +    isActive = virDomainObjIsActive(dom);
> +
> +    if (*flags == VIR_DOMAIN_AFFECT_CURRENT) {

Aargh.  If this is going to be generic, then we have to mask off just
the bits that we care about, to leave the caller free to use the
remaining 30 bits (the code you factored from didn't use the remaining
bits, so they got away with the shortcut).

> +        if (isActive)
> +            *flags = VIR_DOMAIN_AFFECT_LIVE;
> +        else
> +            *flags = VIR_DOMAIN_AFFECT_CONFIG;
> +    }
> +
> +    if (!isActive && (*flags & VIR_DOMAIN_AFFECT_LIVE)) {
> +        virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                             _("domain is not running"));

This should not be INTERNAL_ERROR, but OPERATION_INVALID (the call is
invalid until the domain transitions to the right state).

> +        ret = -1;
> +    }

Once we have an error, we should go to the cleanup section rather than
overwrite it with another error (we don't want to return *persistentDef
if we already reported an error).

> +
> +    if (*flags & VIR_DOMAIN_AFFECT_CONFIG) {
> +        if (!dom->persistent) {
> +            virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                 _("cannot change persistent config of a transient domain"));
> +            ret = -1;

Likewise on the error value.

> +        }
> +        if (!(*persistentDef = virDomainObjGetPersistentDef(caps, dom))) {
> +            virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                             _("Get persistent config failed"));

This one is okay as internal error, though.

> +            ret = -1;
> +        }
> +    }
> +
> +    return ret;
> +}
> +

> +++ b/src/conf/domain_conf.h

Phooey - I ran out of review time mid-patch.  Here's what I'd squash in
(unless passing isactive back to the user means even more changes).
Would you mind sending v2 with this added in?

diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index 12ea12d..e77d824 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -1687,32 +1687,36 @@ virDomainLiveConfigHelperMethod(virCapsPtr caps,

     isActive = virDomainObjIsActive(dom);

-    if (*flags == VIR_DOMAIN_AFFECT_CURRENT) {
+    if ((*flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) ==
+        VIR_DOMAIN_AFFECT_CURRENT) {
         if (isActive)
-            *flags = VIR_DOMAIN_AFFECT_LIVE;
+            *flags |= VIR_DOMAIN_AFFECT_LIVE;
         else
-            *flags = VIR_DOMAIN_AFFECT_CONFIG;
+            *flags |= VIR_DOMAIN_AFFECT_CONFIG;
     }

     if (!isActive && (*flags & VIR_DOMAIN_AFFECT_LIVE)) {
-        virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+        virDomainReportError(VIR_ERR_OPERATION_INVALID, "%s",
                              _("domain is not running"));
-        ret = -1;
+        goto cleanup;
     }

     if (*flags & VIR_DOMAIN_AFFECT_CONFIG) {
         if (!dom->persistent) {
-            virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                                 _("cannot change persistent config of
a transient domain"));
-            ret = -1;
+            virDomainReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                                 _("cannot change persistent config of a "
+                                   "transient domain"));
+            goto cleanup;
         }
         if (!(*persistentDef = virDomainObjGetPersistentDef(caps, dom))) {
             virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                              _("Get persistent config failed"));
-            ret = -1;
+            goto cleanup;
         }
     }

+    ret = 0;
+cleanup:
     return ret;
 }



-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

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