* [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path
@ 2015-08-07 16:08 Roger Pau Monne
2015-08-11 8:57 ` Wei Liu
0 siblings, 1 reply; 8+ messages in thread
From: Roger Pau Monne @ 2015-08-07 16:08 UTC (permalink / raw
To: xen-devel
Cc: Wei Liu, Stefano Stabellini, Ian Jackson, Ian Campbell,
Roger Pau Monne
This is a simple fix to make sure libxl__build_hvm returns an error code in
case of failure.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
I would rather prefer to have it fixed in a proper way like it's done in my
"libxl: fix libxl__build_hvm error handling" as part of the HVMlite series,
but I understand that given the current status of the tree and the
willingness to backport this to stable branches the other approach is going
to be much harder.
---
tools/libxl/libxl_dom.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index e1f11a3..668ce11 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1019,6 +1019,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
return 0;
out:
+ rc = ERROR_FAIL;
return rc;
}
--
1.9.5 (Apple Git-50.3)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path
2015-08-07 16:08 [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path Roger Pau Monne
@ 2015-08-11 8:57 ` Wei Liu
2015-08-11 12:44 ` Ian Campbell
0 siblings, 1 reply; 8+ messages in thread
From: Wei Liu @ 2015-08-11 8:57 UTC (permalink / raw
To: Roger Pau Monne
Cc: xen-devel, Wei Liu, Ian Jackson, Ian Campbell, Stefano Stabellini
On Fri, Aug 07, 2015 at 06:08:25PM +0200, Roger Pau Monne wrote:
> This is a simple fix to make sure libxl__build_hvm returns an error code in
> case of failure.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Though I would like to make commit message clearer.
In 25652f23 ("tools/libxl: detect and avoid conflicts with RDM"), new
code was added to use rc to store libxl function call return value,
which complied to libxl coding style. That patch, however, didn't change
other locations where return value was stored in ret. In the end
libxl__build_hvm could return 0 when it failed.
Explicitly set rc to ERROR_FAIL in error path to fix this. A more
comprehensive fix would be changing all ret to rc, which should be done
when next development window opens.
> ---
> I would rather prefer to have it fixed in a proper way like it's done in my
> "libxl: fix libxl__build_hvm error handling" as part of the HVMlite series,
> but I understand that given the current status of the tree and the
> willingness to backport this to stable branches the other approach is going
> to be much harder.
> ---
> tools/libxl/libxl_dom.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index e1f11a3..668ce11 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1019,6 +1019,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
>
> return 0;
> out:
> + rc = ERROR_FAIL;
> return rc;
> }
>
> --
> 1.9.5 (Apple Git-50.3)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path
2015-08-11 8:57 ` Wei Liu
@ 2015-08-11 12:44 ` Ian Campbell
2015-08-11 12:48 ` Wei Liu
0 siblings, 1 reply; 8+ messages in thread
From: Ian Campbell @ 2015-08-11 12:44 UTC (permalink / raw
To: Wei Liu, Roger Pau Monne; +Cc: xen-devel, Ian Jackson, Stefano Stabellini
On Tue, 2015-08-11 at 09:57 +0100, Wei Liu wrote:
> On Fri, Aug 07, 2015 at 06:08:25PM +0200, Roger Pau Monne wrote:
> > This is a simple fix to make sure libxl__build_hvm returns an error
> > code in
> > case of failure.
> >
>
>
>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
Unfortunately I think this will result in any valid rc's any path happens
to have being discarded in favour of a generic ERROR_FAIL.
If we are going to band aid this for 4.6 then I think setting rc =
ERROR_FAIL just after the libxl__domain_device_construct_rdm error handling
might be better.
Even better would be to put the rc = ERROR_FAIL into the various if (ret)
blocks. I don't think that would be an unacceptably large patch (it's 3-4
sites from what I can see) and it would be closer to heading in the right
direction.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path
2015-08-11 12:44 ` Ian Campbell
@ 2015-08-11 12:48 ` Wei Liu
0 siblings, 0 replies; 8+ messages in thread
From: Wei Liu @ 2015-08-11 12:48 UTC (permalink / raw
To: Ian Campbell
Cc: xen-devel, Stefano Stabellini, Wei Liu, Ian Jackson,
Roger Pau Monne
On Tue, Aug 11, 2015 at 01:44:45PM +0100, Ian Campbell wrote:
> On Tue, 2015-08-11 at 09:57 +0100, Wei Liu wrote:
> > On Fri, Aug 07, 2015 at 06:08:25PM +0200, Roger Pau Monne wrote:
> > > This is a simple fix to make sure libxl__build_hvm returns an error
> > > code in
> > > case of failure.
> > >
> >
> >
> >
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > Cc: Ian Campbell <ian.campbell@citrix.com>
> > > Cc: Wei Liu <wei.liu2@citrix.com>
> >
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
>
> Unfortunately I think this will result in any valid rc's any path happens
> to have being discarded in favour of a generic ERROR_FAIL.
>
Don't worry, this is the original behaviour.
> If we are going to band aid this for 4.6 then I think setting rc =
> ERROR_FAIL just after the libxl__domain_device_construct_rdm error handling
> might be better.
>
> Even better would be to put the rc = ERROR_FAIL into the various if (ret)
> blocks. I don't think that would be an unacceptably large patch (it's 3-4
> sites from what I can see) and it would be closer to heading in the right
> direction.
>
I can do this as well, since Roger is on vacation at the moment.
Wei.
> Ian.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path
@ 2015-08-11 13:48 Wei Liu
2015-08-11 13:55 ` Ian Campbell
2015-08-12 15:49 ` Ian Jackson
0 siblings, 2 replies; 8+ messages in thread
From: Wei Liu @ 2015-08-11 13:48 UTC (permalink / raw
To: xen-devel
Cc: Ian Jackson, Roger Pau Monne, Wei Liu, Ian Campbell,
Stefano Stabellini
In 25652f23 ("tools/libxl: detect and avoid conflicts with RDM"), new
code was added to use rc to store libxl function call return value,
which complied to libxl coding style. That patch, however, didn't change
other locations where return value was stored in ret. In the end
libxl__build_hvm could return 0 when it failed.
Explicitly set rc to ERROR_FAIL in all error paths to fix this.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl_dom.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index e1f11a3..17ae4bd 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -966,12 +966,19 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
ret = libxl__vnuma_build_vmemrange_hvm(gc, domid, info, state, &args);
if (ret) {
LOGEV(ERROR, ret, "hvm build vmemranges failed");
+ rc = ERROR_FAIL;
goto out;
}
ret = libxl__vnuma_config_check(gc, info, state);
- if (ret) goto out;
+ if (ret) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
ret = set_vnuma_info(gc, domid, info, state);
- if (ret) goto out;
+ if (ret) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
args.nr_vmemranges = state->num_vmemranges;
args.vmemranges = libxl__malloc(gc, sizeof(*args.vmemranges) *
@@ -994,6 +1001,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
ret = xc_hvm_build(ctx->xch, domid, &args);
if (ret) {
LOGEV(ERROR, ret, "hvm building failed");
+ rc = ERROR_FAIL;
goto out;
}
@@ -1008,12 +1016,14 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
state->console_domid);
if (ret) {
LOGEV(ERROR, ret, "hvm build set params failed");
+ rc = ERROR_FAIL;
goto out;
}
ret = hvm_build_set_xs_values(gc, domid, &args);
if (ret) {
LOG(ERROR, "hvm build set xenstore values failed (ret=%d)", ret);
+ rc = ERROR_FAIL;
goto out;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path
2015-08-11 13:48 Wei Liu
@ 2015-08-11 13:55 ` Ian Campbell
2015-08-11 13:58 ` Wei Liu
2015-08-12 15:49 ` Ian Jackson
1 sibling, 1 reply; 8+ messages in thread
From: Ian Campbell @ 2015-08-11 13:55 UTC (permalink / raw
To: Wei Liu, xen-devel; +Cc: Roger Pau Monne, Ian Jackson, Stefano Stabellini
On Tue, 2015-08-11 at 14:48 +0100, Wei Liu wrote:
> In 25652f23 ("tools/libxl: detect and avoid conflicts with RDM"), new
> code was added to use rc to store libxl function call return value,
> which complied to libxl coding style. That patch, however, didn't change
> other locations where return value was stored in ret. In the end
> libxl__build_hvm could return 0 when it failed.
>
> Explicitly set rc to ERROR_FAIL in all error paths to fix this.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
You missed the path from libxl__domain_firmware, which incorrectly relies
on rc being already initialised by the declaration (which per CODING_STYLE
ought to be removed too).
However perhaps you prefer to leave those other two hunks until 4.7 and
this patch is at least an improvement of sorts so:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path
2015-08-11 13:55 ` Ian Campbell
@ 2015-08-11 13:58 ` Wei Liu
0 siblings, 0 replies; 8+ messages in thread
From: Wei Liu @ 2015-08-11 13:58 UTC (permalink / raw
To: Ian Campbell
Cc: xen-devel, Roger Pau Monne, Wei Liu, Ian Jackson,
Stefano Stabellini
On Tue, Aug 11, 2015 at 02:55:41PM +0100, Ian Campbell wrote:
> On Tue, 2015-08-11 at 14:48 +0100, Wei Liu wrote:
> > In 25652f23 ("tools/libxl: detect and avoid conflicts with RDM"), new
> > code was added to use rc to store libxl function call return value,
> > which complied to libxl coding style. That patch, however, didn't change
> > other locations where return value was stored in ret. In the end
> > libxl__build_hvm could return 0 when it failed.
> >
> > Explicitly set rc to ERROR_FAIL in all error paths to fix this.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
> You missed the path from libxl__domain_firmware, which incorrectly relies
> on rc being already initialised by the declaration (which per CODING_STYLE
> ought to be removed too).
>
> However perhaps you prefer to leave those other two hunks until 4.7 and
> this patch is at least an improvement of sorts so:
>
Yes. Roger already has a patch for 4.7 to fix error handling in this
function.
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Thanks
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path
2015-08-11 13:48 Wei Liu
2015-08-11 13:55 ` Ian Campbell
@ 2015-08-12 15:49 ` Ian Jackson
1 sibling, 0 replies; 8+ messages in thread
From: Ian Jackson @ 2015-08-12 15:49 UTC (permalink / raw
To: Wei Liu; +Cc: xen-devel, Roger Pau Monne, Ian Campbell, Stefano Stabellini
Wei Liu writes ("[PATCH for-4.6] libxl: fix libxl__build_hvm error code return path"):
> In 25652f23 ("tools/libxl: detect and avoid conflicts with RDM"), new
> code was added to use rc to store libxl function call return value,
> which complied to libxl coding style. That patch, however, didn't change
> other locations where return value was stored in ret. In the end
> libxl__build_hvm could return 0 when it failed.
>
> Explicitly set rc to ERROR_FAIL in all error paths to fix this.
...
> ret = libxl__vnuma_build_vmemrange_hvm(gc, domid, info, state, &args);
> if (ret) {
Why not change all the assignemnts to ret to assignments to rc, and
abolish ret ?
> LOGEV(ERROR, ret, "hvm build vmemranges failed");
> + rc = ERROR_FAIL;
Certainly this is wrong because it smashes all errors into ERROR_FAIL.
Ian.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-08-12 15:49 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-07 16:08 [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path Roger Pau Monne
2015-08-11 8:57 ` Wei Liu
2015-08-11 12:44 ` Ian Campbell
2015-08-11 12:48 ` Wei Liu
-- strict thread matches above, loose matches on Subject: below --
2015-08-11 13:48 Wei Liu
2015-08-11 13:55 ` Ian Campbell
2015-08-11 13:58 ` Wei Liu
2015-08-12 15:49 ` Ian Jackson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).