[PATCH for 4.5] xl: Return proper error codes for block-attach and block-detach

[PATCH for 4.5] xl: Return proper error codes for block-attach and block-detach

[George Dunlap]

Return proper error codes on failure so that scripts can tell whether
the command completed properly or not.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Konrad Wilk <konrad.wilk@oracle.com>

Release justification: This is a bug fix.  It's a fairly minor one,
but it's also a very simple one.
---
 tools/libxl/xl_cmdimpl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 3c9f146..b2abe4f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -6383,6 +6383,7 @@ int main_blockattach(int argc, char **argv)
 
     if (libxl_device_disk_add(ctx, fe_domid, &disk, 0)) {
         fprintf(stderr, "libxl_device_disk_add failed.\n");
+        return 1;
     }
     return 0;
 }
@@ -6444,6 +6445,7 @@ int main_blockdetach(int argc, char **argv)
     rc = libxl_device_disk_remove(ctx, domid, &disk, 0);
     if (rc) {
         fprintf(stderr, "libxl_device_disk_remove failed.\n");
+        return 1;
     }
     libxl_device_disk_dispose(&disk);
     return rc;
-- 
1.9.1

Re: [PATCH for 4.5] xl: Return proper error codes for block-attach and block-detach

[Wei Liu]

On Wed, Nov 12, 2014 at 05:31:33PM +0000, George Dunlap wrote:
> Return proper error codes on failure so that scripts can tell whether
> the command completed properly or not.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> CC: Konrad Wilk <konrad.wilk@oracle.com>
> 
> Release justification: This is a bug fix.  It's a fairly minor one,
> but it's also a very simple one.
> ---
>  tools/libxl/xl_cmdimpl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 3c9f146..b2abe4f 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -6383,6 +6383,7 @@ int main_blockattach(int argc, char **argv)
>  
>      if (libxl_device_disk_add(ctx, fe_domid, &disk, 0)) {
>          fprintf(stderr, "libxl_device_disk_add failed.\n");
> +        return 1;
>      }
>      return 0;
>  }
> @@ -6444,6 +6445,7 @@ int main_blockdetach(int argc, char **argv)
>      rc = libxl_device_disk_remove(ctx, domid, &disk, 0);
>      if (rc) {
>          fprintf(stderr, "libxl_device_disk_remove failed.\n");
> +        return 1;
>      }
>      libxl_device_disk_dispose(&disk);
>      return rc;
> -- 
> 1.9.1

Re: [PATCH for 4.5] xl: Return proper error codes for block-attach and block-detach

[Ian Campbell]

On Wed, 2014-11-12 at 17:31 +0000, George Dunlap wrote:
> @@ -6444,6 +6445,7 @@ int main_blockdetach(int argc, char **argv)
>      rc = libxl_device_disk_remove(ctx, domid, &disk, 0);
>      if (rc) {
>          fprintf(stderr, "libxl_device_disk_remove failed.\n");
> +        return 1;
>      }
>      libxl_device_disk_dispose(&disk);
>      return rc;

This one seems odd to me, you have inadvertently arranged to skip the
disk dispose and the old code returned non-zero in the failure case
already, since it returns rc.

If it is important to return 0 or 1 then perhaps the last line should
just be:
        return rc ? 1 : 0;
        
Or maybe the ultimate caller (i.e. the xl command dispatcher) ought to
be translating libxl ERROR_FOO into suitable process exit codes (perhaps
as simplistically as suggested above).

Skipping the dispose is a memory leak, albeit in a process which is just
about to die, but we like to try and keep xl "valgrind clean" so far as
possible such that leaks in the underlying libxl stand out.

Ian.

Re: [PATCH for 4.5] xl: Return proper error codes for block-attach and block-detach

[George Dunlap]

On Wed, Nov 12, 2014 at 5:31 PM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> Return proper error codes on failure so that scripts can tell whether
> the command completed properly or not.

How about changing this to something like:

---
Return proper error codes on failure so that scripts can tell whether
the command completed properly or not.

This is not a proper fix, since it fails to call
libxl_device_disk_dispose() on the error path.  But a proper fix
requires some refactoring, so given where we are in the release
process, it's better to have a fix that is simple and obvious, and do
the refactoring once the next development window opens up.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---

 -George

Re: [PATCH for 4.5] xl: Return proper error codes for block-attach and block-detach

[Ian Campbell]

On Thu, 2014-11-20 at 16:06 +0000, George Dunlap wrote:
> On Wed, Nov 12, 2014 at 5:31 PM, George Dunlap
> <george.dunlap@eu.citrix.com> wrote:
> > Return proper error codes on failure so that scripts can tell whether
> > the command completed properly or not.
> 
> How about changing this to something like:
> 
> ---
> Return proper error codes on failure so that scripts can tell whether
> the command completed properly or not.
> 
> This is not a proper fix, since it fails to call
> libxl_device_disk_dispose() on the error path.  But a proper fix
> requires some refactoring, so given where we are in the release
> process, it's better to have a fix that is simple and obvious, and do
> the refactoring once the next development window opens up.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Looks good, I kept the single line title the same and replaced the body
with the above, which I think is what you intended.

Konrad acked taking this patch for 4.5 in a reply to v2, so I have
pushed.

Ian.