On Thu, Nov 20, 2014 at 03:47:04PM +, Ian Campbell wrote:
On Mon, 2014-11-17 at 12:36 +, George Dunlap wrote:
On 11/14/2014 11:12 AM, Ian Campbell wrote:
On Thu, 2014-11-13 at 19:04 +, George Dunlap wrote:
Return proper error codes on failure so that scripts can tell whether
the command completed properly or not.
Also while we're at it, normalize init and dispose of
libxl_device_disk structures. This means calling init and dispose in
the actual functions where they are declared.
This in turn means changing parse_disk_config_multistring() to not
call libxl_device_disk_init. And that in turn means changes to
callers of parse_disk_config().
It also means removing libxl_device_disk_init() from
libxl.c:libxl_vdev_to_device_disk(). This is only called from
xl_cmdimpl.c.
Signed-off-by: George Dunlap george.dun...@eu.citrix.com
---
CC: Ian Campbell ian.campb...@citrix.com
CC: Ian Jackson ian.jack...@citrix.com
CC: Wei Liu wei.l...@citrix.com
CC: Konrad Wilk konrad.w...@oracle.com
Release justification: This is a bug fix. It's a fairly minor one,
but it's also a very simple one.
v2:
- Restructure functions to make sure init and dispose are properly
called.
Sadly this bit has somewhat reduced the truth of the second half of your
release justification, since the patch is a fair bit more subtle though.
Although IMHO it hasn't invalidated the case for taking the patch for
4.5 (modulo comments below).
Well, I think we can take the hacky short-term fix I posted before,
which is simple, and do a proper fix after the 4.6 dev window opens up;
or we can do a more complete fix now.
Specifically the hacky short-term fix is
1415813493-25330-1-git-send-email-george.dun...@eu.citrix.com ?
I could live with that, perhaps with the commit log explaining that a
little.
Konrad?
hands George the band-aid tape
Release-Acked-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com
Or, if the valgrind thing is really important, I could use the change
you suggested, and do return rc ? 1 : 0; but I really think that's
going in the wrong direction...
-George
---
tools/libxl/libxl.c | 2 --
tools/libxl/xl_cmdimpl.c | 28 +---
2 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index f7961f6..d9c4ce7 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2611,8 +2611,6 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx,
uint32_t domid,
if (devid 0)
return ERROR_INVAL;
-libxl_device_disk_init(disk);
_init functions are idempotent, so this is harmless, I think. Existing
callers (including things which aren't xl) might be relying on it.
Outside of a freeze period that might be acceptable (those callers are
buggy) but since you are asking for a freeze exception I think this may
be going a step too far in cleaning things up.
In terms of other callers you've missed
tools/ocaml/libs/xl/xenlight_stubs.c (which appears buggy to me) in
tree, plus whatever use e.g. libvirt makes of it.
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 3c9f146..25af715 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -485,8 +485,6 @@ static void parse_disk_config_multistring(XLU_Config
**config,
{
int e;
-libxl_device_disk_init(disk);
Likewise here, although to a lesser extent since this is xl not libxl.
@@ -6378,13 +6382,17 @@ int main_blockattach(int argc, char **argv)
printf(disk: %s\n, json);
free(json);
if (ferror(stdout) || fflush(stdout)) { perror(stdout);
exit(-1); }
-return 0;
You should set rc = 0 here rather than initing it at declaration to
catch error paths which don't set the result. (we aren't very consistent
about this in the code today so I'm only mentioning it because other
changes seem to be needed, if that turns out not to be the case and
there's no need for v3 then this shouldn't block acceptance)
+goto out;
I'm not 100% convinced by the use of the goto out error handling style
for a success path, but it's probably better than duplicating the exit
path or adding !dryrun checks to all the following operations. Ian,
since you recently posted updated coding style for things around this,
what do you think?
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
