RE: [Xen-devel] [PATCH] Clear device-model information when destroying an HVM domain with stubdom-v3

["Jun Zhu (Intern)" <Jun.Zhu@xxxxxxxxxx>]

Hi,

I am sorry that the last version was made on a wrong version.
Thanks for your suggesion. I use goto out in both wrong condition, and remove 
the unnecessory logging. The vim tabs has been converted to 4 spaces.


exporting patch:
# HG changeset patch
# User Jun Zhu <Jun.Zhu@xxxxxxxxxx>
# Date 1284113407 -3600
# Node ID e87bef8e5070d72ebcaacb420a2a36af622b8642
# Parent  1831912d4109731e78c01be40ec70b5fae804d30
Clear device-model information when destroying an HVM domain with stubdom. V3.

diff -r 1831912d4109 -r e87bef8e5070 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Thu Sep 02 19:12:42 2010 +0100
+++ b/tools/libxl/libxl.c       Fri Sep 10 11:10:07 2010 +0100
@@ -899,21 +899,27 @@
         }
         XL_LOG(ctx, XL_LOG_ERROR, "Device model is a stubdom, domid=%d\n", 
stubdomid);
         ret = libxl_domain_destroy(ctx, stubdomid, 0);
-        goto out;
+        if (ret)
+            goto out;
     }
-    xs_rm(ctx->xsh, XBT_NULL, libxl_sprintf(&gc, 
"/local/domain/0/device-model/%d", domid));
-
-    ret = kill(atoi(pid), SIGHUP);
-    if (ret < 0 && errno == ESRCH) {
-        XL_LOG(ctx, XL_LOG_DEBUG, "Device Model already exited");
-        ret = 0;
-    } else if (ret == 0) {
-        XL_LOG(ctx, XL_LOG_DEBUG, "Device Model signaled");
-        ret = 0;
-    } else {
-        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "failed to kill Device Model [%d]",
-                     atoi(pid));
+    else
+    {
+        ret = kill(atoi(pid), SIGHUP);
+        if (ret < 0 && errno == ESRCH) {
+            XL_LOG(ctx, XL_LOG_DEBUG, "Device Model already exited");
+            ret = 0;
+        } else if (ret == 0) {
+            XL_LOG(ctx, XL_LOG_DEBUG, "Device Model signaled");
+            ret = 0;
+        } else {
+            XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "failed to kill Device Model [%d]",
+                    atoi(pid));
+            goto out;
+        }
     }
+    xs_rm(ctx->xsh, XBT_NULL,
+            libxl_sprintf(&gc, "/local/domain/0/device-model/%d", domid));
+
 out:
     libxl_free_all(&gc);
     return ret;


Jun Zhu
Citrix Systems UK
________________________________________
From: Ian Jackson
Sent: 09 September 2010 13:15
To: Jun Zhu (Intern)
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: RE: [Xen-devel] [PATCH] Clear device-model information when destroying 
an HVM domain with stubdom

Jun Zhu (Intern) writes ("RE: [Xen-devel] [PATCH] Clear device-model 
information when destroying an HVM domain with stubdom"):
> I make a new path from the newest unstable version.

Thanks.  Like your previous patch, this seems to have some whitespace
and formatting errors.  Also, please try to keep your lines to 75
characters or less.

>          ret = libxl_domain_destroy(ctx, stubdomid, 0);
> -        goto out;
> +        if (!ret)
> +             {
> +                     XL_LOG(ctx, XL_LOG_ERROR, "Destroy stubdom failed, 
> domid=%d\n", stubdomid);

Surely libxl_domain_destroy will have already logged errors so there
is no need to do so again ?  Also I think you have the sense of the
error test reversed: ret==0 is success so just
  if (ret) goto out;
will do.

> +                     ret = 0;
> +             } else if (ret == 0) {
...
> +                     XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "failed to kill Device 
> Model [%d]",
> +                                     atoi(pid));
> +             }
> +     }
> +     if (!ret)
> +         xs_rm(ctx->xsh, XBT_NULL, libxl_sprintf(&gc, 
> "/local/domain/0/device-model/%d", domid));
>  out:

Your logic here seems inconsistent.  In the one case you skip the
removal by the use of "goto out" and in the other you test "ret".
Normally I think the "goto out" approach is probably better here.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel