[PATCH for-4.6] libxl: fix libxl__build_hvm error code return path

[PATCH for-4.6] libxl: fix libxl__build_hvm error code return path

[Wei Liu]

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

Re: [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path

[Ian Campbell]

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>

Re: [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path

[Wei Liu]

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

Re: [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path

[Ian Jackson]

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.

[PATCH for-4.6] libxl: fix libxl__build_hvm error code return path

[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

Re: [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path

[Wei Liu]

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)

Re: [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path

[Ian Campbell]

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

Re: [PATCH for-4.6] libxl: fix libxl__build_hvm error code return path

[Wei Liu]

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.