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

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@citrix.com>
# 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@lists.xensource.com
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@lists.xensource.com
http://lists.xensource.com/xen-devel

Jun Zhu (Intern) writes ("RE: [Xen-devel] [PATCH] Clear device-model
information when destroying an HVM domain with
stubdom-v3"):
> 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.
Thanks.  You still need to put { and } on the same line as the else :-).

Also, more seriously:
> +        } else {
> +            XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "failed to kill Device
Model [%d]",
> +                    atoi(pid));
> +            goto out;
This should set ret to something appropriate.  ERROR_FAIL, probably.

Ian.

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