Ian Campbell
2012-Jul-02 10:15 UTC
[PATCH] xl: ensure handle_domain & preserve_domain update the global domid not a shadow
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1341224092 -3600
# Node ID e3f4595bdbd87ac71151f01ed2be36e45636458b
# Parent b625271894d40aecd9599bf448129e14d0af0122
xl: ensure handle_domain & preserve_domain update the global domid not a
shadow
xl keeps the current domid in a global variable, however it also has various
functions (including the two above) which take a domid parameter which shadows
this.
This fixes the issue introduced by 25563:dbf54d93ac40 "xl: initialise domid
to
an explicitly invalid value" but does not tackle the wider problem.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
I looked at fixing this by both removing the global domid or renaming the
shadowing arguments but both were pretty invasive and I wanted to get this
test failure fixed. Compiling tools/libxl with Wshadow shows that this is far
from the only instance of this kind of problem. We should look to enable this
warning in 4.3.
diff -r b625271894d4 -r e3f4595bdbd8 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Mon Jul 02 10:39:23 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c Mon Jul 02 11:14:52 2012 +0100
@@ -1328,8 +1328,9 @@ static void reload_domain_config(uint32_
}
/* Returns 1 if domain should be restarted,
- * 2 if domain should be renamed then restarted, or 0 */
-static int handle_domain_death(uint32_t domid,
+ * 2 if domain should be renamed then restarted, or 0
+ * Can update r_domid if doain is destroyed etc */
+static int handle_domain_death(uint32_t *r_domid,
libxl_event *event,
uint8_t **config_data, int *config_len,
libxl_domain_config *d_config)
@@ -1372,7 +1373,7 @@ static int handle_domain_death(uint32_t
LOG("failed to construct core dump path");
} else {
LOG("dumping core to %s", corefile);
- rc=libxl_domain_core_dump(ctx, domid, corefile, NULL);
+ rc=libxl_domain_core_dump(ctx, *r_domid, corefile, NULL);
if (rc) LOG("core dump failed (rc=%d).", rc);
}
/* No point crying over spilled milk, continue on failure. */
@@ -1388,19 +1389,20 @@ static int handle_domain_death(uint32_t
break;
case LIBXL_ACTION_ON_SHUTDOWN_RESTART_RENAME:
- reload_domain_config(domid, config_data, config_len);
+ reload_domain_config(*r_domid, config_data, config_len);
restart = 2;
break;
case LIBXL_ACTION_ON_SHUTDOWN_RESTART:
- reload_domain_config(domid, config_data, config_len);
+ reload_domain_config(*r_domid, config_data, config_len);
restart = 1;
/* fall-through */
case LIBXL_ACTION_ON_SHUTDOWN_DESTROY:
- LOG("Domain %d needs to be cleaned up: destroying the
domain", domid);
- libxl_domain_destroy(ctx, domid);
- domid = INVALID_DOMID;
+ LOG("Domain %d needs to be cleaned up: destroying the
domain",
+ *r_domid);
+ libxl_domain_destroy(ctx, *r_domid);
+ *r_domid = INVALID_DOMID;
break;
case LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_DESTROY:
@@ -1430,7 +1432,8 @@ static void replace_string(char **str, c
}
-static int preserve_domain(uint32_t domid, libxl_event *event,
+/* Preserve a copy of a domain under a new name. Updates *r_domid */
+static int preserve_domain(uint32_t *r_domid, libxl_event *event,
libxl_domain_config *d_config)
{
time_t now;
@@ -1460,14 +1463,16 @@ static int preserve_domain(uint32_t domi
libxl_uuid_generate(&new_uuid);
- LOG("Preserving domain %d %s with suffix%s", domid,
d_config->c_info.name, stime);
- rc = libxl_domain_preserve(ctx, domid, &d_config->c_info, stime,
new_uuid);
+ LOG("Preserving domain %d %s with suffix%s",
+ *r_domid, d_config->c_info.name, stime);
+ rc = libxl_domain_preserve(ctx, *r_domid, &d_config->c_info,
+ stime, new_uuid);
/*
- * Although domid still exists it is no longer the one we are concerned
- * with.
+ * Although the domain still exists it is no longer the one we are
+ * concerned with.
*/
- domid = INVALID_DOMID;
+ *r_domid = INVALID_DOMID;
return rc == 0 ? 1 : 0;
}
@@ -1921,11 +1926,11 @@ start:
LOG("Domain %d has shut down, reason code %d 0x%x",
domid,
event->u.domain_shutdown.shutdown_reason,
event->u.domain_shutdown.shutdown_reason);
- switch (handle_domain_death(domid, event,
+ switch (handle_domain_death(&domid, event,
(uint8_t **)&config_data,
&config_len,
&d_config)) {
case 2:
- if (!preserve_domain(domid, event, &d_config)) {
+ if (!preserve_domain(&domid, event, &d_config)) {
/* If we fail then exit leaving the old domain in place. */
ret = -1;
goto out;
Roger Pau Monné
2012-Jul-02 10:32 UTC
Re: [PATCH] xl: ensure handle_domain & preserve_domain update the global domid not a shadow
On Mon, Jul 2, 2012 at 11:15 AM, Ian Campbell <ian.campbell@citrix.com> wrote:> # HG changeset patch > # User Ian Campbell <ian.campbell@citrix.com> > # Date 1341224092 -3600 > # Node ID e3f4595bdbd87ac71151f01ed2be36e45636458b > # Parent b625271894d40aecd9599bf448129e14d0af0122 > xl: ensure handle_domain & preserve_domain update the global domid not a shadow > > xl keeps the current domid in a global variable, however it also has various > functions (including the two above) which take a domid parameter which shadows > this. > > This fixes the issue introduced by 25563:dbf54d93ac40 "xl: initialise domid to > an explicitly invalid value" but does not tackle the wider problem. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>Acked-by: Roger Pau Monne <roger.pau@citrix.com> (just a minor typo)> --- > > I looked at fixing this by both removing the global domid or renaming the > shadowing arguments but both were pretty invasive and I wanted to get this > test failure fixed. Compiling tools/libxl with Wshadow shows that this is far > from the only instance of this kind of problem. We should look to enable this > warning in 4.3.Yes, fixing this is 4.3 material I think.> diff -r b625271894d4 -r e3f4595bdbd8 tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c Mon Jul 02 10:39:23 2012 +0100 > +++ b/tools/libxl/xl_cmdimpl.c Mon Jul 02 11:14:52 2012 +0100 > @@ -1328,8 +1328,9 @@ static void reload_domain_config(uint32_ > } > > /* Returns 1 if domain should be restarted, > - * 2 if domain should be renamed then restarted, or 0 */ > -static int handle_domain_death(uint32_t domid, > + * 2 if domain should be renamed then restarted, or 0 > + * Can update r_domid if doain is destroyed etc */domain> +static int handle_domain_death(uint32_t *r_domid, > libxl_event *event, > uint8_t **config_data, int *config_len, > libxl_domain_config *d_config)