Jun Zhu (Intern)
2010-Sep-06 09:40 UTC
[Xen-devel] [PATCH] Clear device-model information when destroying an HVM domain with stubdom
Hi,
This patch clears the device-model information from xenstore when destroying an
HVM domain with stubdom.
Signed-off-by: Jun Zhu <Jun.Zhu@citrix.com>
diff -r 0d8541707371 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Mon Sep 06 09:58:16 2010 +0100
+++ b/tools/libxl/libxl.c Mon Sep 06 10:37:29 2010 +0100
@@ -834,6 +834,8 @@
char *pid;
int ret;
+ xs_rm(ctx->xsh, XBT_NULL, libxl_sprintf(ctx,
"/local/domain/0/device-model/%d", domid));
+
pid = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx,
"/local/domain/%d/image/device-model-pid", domid));
if (!pid) {
int stubdomid = libxl_get_stubdom_id(ctx, domid);
@@ -844,7 +846,6 @@
XL_LOG(ctx, XL_LOG_ERROR, "Device model is a stubdom,
domid=%d\n", stubdomid);
return libxl_domain_destroy(ctx, stubdomid, 0);
}
- xs_rm(ctx->xsh, XBT_NULL, libxl_sprintf(ctx,
"/local/domain/0/device-model/%d", domid));
ret = kill(atoi(pid), SIGHUP);
if (ret < 0 && errno == ESRCH) {
Jun Zhu
Citrix Systems UK
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2010-Sep-08 13:51 UTC
Re: [Xen-devel] [PATCH] Clear device-model information when destroying an HVM domain with stubdom
Jun Zhu (Intern) writes ("[Xen-devel] [PATCH] Clear device-model
information when destroying an HVM domain with
stubdom"):> This patch clears the device-model information from xenstore when
> destroying an HVM domain with stubdom.
I don''t think this patch is correct.
> + xs_rm(ctx->xsh, XBT_NULL, libxl_sprintf(ctx,
"/local/domain/0/device-model/%d", domid));
> +
This removes the information from xenstore before the process is
killed. These things should be done in the opposite order, so that we
are never (even briefly) in a situation with a dm (whether stubdom or
dom0 process) which is not mentioned in xenstore.
The code already has this bug in the dom0 process case.
> pid = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx,
"/local/domain/%d/image/device-model-pid", domid));
> if (!pid) {
> int stubdomid = libxl_get_stubdom_id(ctx, domid);
> @@ -844,7 +846,6 @@
> XL_LOG(ctx, XL_LOG_ERROR, "Device model is a stubdom,
domid=%d\n", stubdomid);
> return libxl_domain_destroy(ctx, stubdomid, 0);
> }
> - xs_rm(ctx->xsh, XBT_NULL, libxl_sprintf(ctx,
"/local/domain/0/device-model/%d", domid));
>
> ret = kill(atoi(pid), SIGHUP);
> if (ret < 0 && errno == ESRCH) {
The right answer is probably to move the xs_rm until _after_ the kill
is dealt with, and to put the kill section in an else branch of the
if, and remove the return in the stubdom case and replace it with an
ordinary error check.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jun Zhu (Intern)
2010-Sep-08 15:16 UTC
RE: [Xen-devel] [PATCH] Clear device-model information when destroying an HVM domain with stubdom
Your suggestion is absolutely right, though it needs more modification. I will
try to make a new patch.
Jun Zhu
Citrix Systems UK
________________________________________
From: Ian Jackson
Sent: 08 September 2010 09:51
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 ("[Xen-devel] [PATCH] Clear device-model
information when destroying an HVM domain with
stubdom"):> This patch clears the device-model information from xenstore when
> destroying an HVM domain with stubdom.
I don''t think this patch is correct.
> + xs_rm(ctx->xsh, XBT_NULL, libxl_sprintf(ctx,
"/local/domain/0/device-model/%d", domid));
> +
This removes the information from xenstore before the process is
killed. These things should be done in the opposite order, so that we
are never (even briefly) in a situation with a dm (whether stubdom or
dom0 process) which is not mentioned in xenstore.
The code already has this bug in the dom0 process case.
> pid = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx,
"/local/domain/%d/image/device-model-pid", domid));
> if (!pid) {
> int stubdomid = libxl_get_stubdom_id(ctx, domid);
> @@ -844,7 +846,6 @@
> XL_LOG(ctx, XL_LOG_ERROR, "Device model is a stubdom,
domid=%d\n", stubdomid);
> return libxl_domain_destroy(ctx, stubdomid, 0);
> }
> - xs_rm(ctx->xsh, XBT_NULL, libxl_sprintf(ctx,
"/local/domain/0/device-model/%d", domid));
>
> ret = kill(atoi(pid), SIGHUP);
> if (ret < 0 && errno == ESRCH) {
The right answer is probably to move the xs_rm until _after_ the kill
is dealt with, and to put the kill section in an else branch of the
if, and remove the return in the stubdom case and replace it with an
ordinary error check.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jun Zhu (Intern)
2010-Sep-08 19:50 UTC
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.
exporting patch:
# HG changeset patch
# User Jun Zhu <Jun.Zhu@citrix.com>
# Date 1283975147 -3600
# Node ID d218c93a1fb7d7a532e1cc324e0cf45a3b96b911
# Parent c54e3f3df3ed2edf4e09a004267df81deb999cc5
Make libxl remove device model infomation when destroying a hvm with stubdom.
diff -r c54e3f3df3ed -r d218c93a1fb7 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Wed Sep 08 20:16:48 2010 +0100
+++ b/tools/libxl/libxl.c Wed Sep 08 20:45:47 2010 +0100
@@ -897,21 +897,28 @@
}
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)
+ {
+ XL_LOG(ctx, XL_LOG_ERROR, "Destroy stubdom failed, domid=%d\n",
stubdomid);
+ 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));
+ }
+ }
+ if (!ret)
+ 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: 08 September 2010 09:51
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 ("[Xen-devel] [PATCH] Clear device-model
information when destroying an HVM domain with
stubdom"):> This patch clears the device-model information from xenstore when
> destroying an HVM domain with stubdom.
I don''t think this patch is correct.
> + xs_rm(ctx->xsh, XBT_NULL, libxl_sprintf(ctx,
"/local/domain/0/device-model/%d", domid));
> +
This removes the information from xenstore before the process is
killed. These things should be done in the opposite order, so that we
are never (even briefly) in a situation with a dm (whether stubdom or
dom0 process) which is not mentioned in xenstore.
The code already has this bug in the dom0 process case.
> pid = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx,
"/local/domain/%d/image/device-model-pid", domid));
> if (!pid) {
> int stubdomid = libxl_get_stubdom_id(ctx, domid);
> @@ -844,7 +846,6 @@
> XL_LOG(ctx, XL_LOG_ERROR, "Device model is a stubdom,
domid=%d\n", stubdomid);
> return libxl_domain_destroy(ctx, stubdomid, 0);
> }
> - xs_rm(ctx->xsh, XBT_NULL, libxl_sprintf(ctx,
"/local/domain/0/device-model/%d", domid));
>
> ret = kill(atoi(pid), SIGHUP);
> if (ret < 0 && errno == ESRCH) {
The right answer is probably to move the xs_rm until _after_ the kill
is dealt with, and to put the kill section in an else branch of the
if, and remove the return in the stubdom case and replace it with an
ordinary error check.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2010-Sep-09 17:15 UTC
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