[PATCH] xl: log an error if libxl_cpupool_destroy() fails

[PATCH] xl: log an error if libxl_cpupool_destroy() fails

[Dario Faggioli]

In fact, right now, failing at destroying a cpupool is just
not reported to the user in any explicit way. Log an error,
as it is customary for xl in these cases.

While there, take the chance to turn a couple of xl exit
codes into EXIT_[SUCCESS|FAILURE], as discussed and agreed
here:

 http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg01336.html
 http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg01341.html

Signed-off-by: Dario Faggioli <dario.faggioli@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>
Cc: Juergen Gross <jgross@suse.com>
---
 tools/libxl/xl_cmdimpl.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 365798b..5a5f959 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7581,13 +7581,15 @@ int main_cpupooldestroy(int argc, char **argv)
     if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
         !libxl_cpupoolid_is_valid(ctx, poolid)) {
         fprintf(stderr, "unknown cpupool '%s'\n", pool);
-        return 1;
+        return EXIT_FAILURE;
     }
 
-    if (libxl_cpupool_destroy(ctx, poolid))
-        return 1;
+    if (libxl_cpupool_destroy(ctx, poolid)) {
+        fprintf(stderr, "Can't destroy cpupool '%s'\n", pool);
+        return EXIT_FAILURE;
+    }
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_cpupoolrename(int argc, char **argv)

Re: [PATCH] xl: log an error if libxl_cpupool_destroy() fails

[Juergen Gross]

On 10/22/2015 07:14 PM, Dario Faggioli wrote:
> In fact, right now, failing at destroying a cpupool is just
> not reported to the user in any explicit way. Log an error,
> as it is customary for xl in these cases.
>
> While there, take the chance to turn a couple of xl exit
> codes into EXIT_[SUCCESS|FAILURE], as discussed and agreed
> here:
>
>   http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg01336.html
>   http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg01341.html
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.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>
> Cc: Juergen Gross <jgross@suse.com>
> ---
>   tools/libxl/xl_cmdimpl.c |   10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 365798b..5a5f959 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -7581,13 +7581,15 @@ int main_cpupooldestroy(int argc, char **argv)
>       if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
>           !libxl_cpupoolid_is_valid(ctx, poolid)) {
>           fprintf(stderr, "unknown cpupool '%s'\n", pool);
> -        return 1;
> +        return EXIT_FAILURE;
>       }
>
> -    if (libxl_cpupool_destroy(ctx, poolid))
> -        return 1;
> +    if (libxl_cpupool_destroy(ctx, poolid)) {
> +        fprintf(stderr, "Can't destroy cpupool '%s'\n", pool);
> +        return EXIT_FAILURE;
> +    }
>
> -    return 0;
> +    return EXIT_SUCCESS;
>   }
>
>   int main_cpupoolrename(int argc, char **argv)
>
>

Re: [PATCH] xl: log an error if libxl_cpupool_destroy() fails

[Wei Liu]

On Thu, Oct 22, 2015 at 07:14:20PM +0200, Dario Faggioli wrote:
> In fact, right now, failing at destroying a cpupool is just
> not reported to the user in any explicit way. Log an error,
> as it is customary for xl in these cases.
> 
> While there, take the chance to turn a couple of xl exit
> codes into EXIT_[SUCCESS|FAILURE], as discussed and agreed
> here:
> 
>  http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg01336.html
>  http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg01341.html
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

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

Re: [PATCH] xl: log an error if libxl_cpupool_destroy() fails

[Ian Campbell]

On Fri, 2015-10-23 at 09:43 +0100, Wei Liu wrote:
> On Thu, Oct 22, 2015 at 07:14:20PM +0200, Dario Faggioli wrote:
> > In fact, right now, failing at destroying a cpupool is just
> > not reported to the user in any explicit way. Log an error,
> > as it is customary for xl in these cases.
> > 
> > While there, take the chance to turn a couple of xl exit
> > codes into EXIT_[SUCCESS|FAILURE], as discussed and agreed
> > here:
> > 
> >  http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg01336.h
> > tml
> >  http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg01341.h
> > tml
> > 
> > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Looking at those links, I'm not sure that either you or myself was thinking
of using EXIT_* as function return values, just that the eventual process
exit would become one of those values instead of some negative number.
Although the thread doesn't look like it is too clear if it is talking
about return values from functions vs. process exit codes.

I think what I would have been expecting is for the xl internal error code
would become EXIT_* either in the call to exit() or the return from main
instead of being the process exit code directly.

Seeing "return EXIT_FOO" outside of a main function seems rather strange to
me.

Wei, you acked so maybe you disagree?

Ian.

Re: [PATCH] xl: log an error if libxl_cpupool_destroy() fails

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2354 bytes --]

On Fri, 2015-10-23 at 15:09 +0100, Ian Campbell wrote:
> On Fri, 2015-10-23 at 09:43 +0100, Wei Liu wrote:

> Looking at those links, I'm not sure that either you or myself was
> thinking
> of using EXIT_* as function return values, just that the eventual
> process
> exit would become one of those values instead of some negative
> number.
>
Exactly.

> Although the thread doesn't look like it is too clear if it is
> talking
> about return values from functions vs. process exit codes.
> 
Well, independently from the thread, I certainly meant that I think
EXIT_* should be used as process exit status, not as internal
functions' return value.

> I think what I would have been expecting is for the xl internal error
> code
> would become EXIT_* either in the call to exit() or the return from
> main
> instead of being the process exit code directly.
> 
But, in this specific case, and in cases of main_foo() functions in
xl_cmdimpl.c, it's exactly like that, isn't it?

    ...
    if (cspec) {
        if (dryrun_only && !cspec->can_dryrun) {
            fprintf(stderr, "command does not implement -N (dryrun) option\n");
            ret = 1;
            goto xit;
        }
        ret = cspec->cmd_impl(argc, argv);
    } else if (!strcmp(cmd, "help")) {
        help(argv[1]);
        ret = 0;
    } else {
        fprintf(stderr, "command not implemented\n");
        ret = 1;
    }

 xit:
    return ret;
}

(from main() in xl.c)

> Seeing "return EXIT_FOO" outside of a main function seems rather
> strange to
> me.
> 
Well, same here. Except, given xl architecture, I was considering
main_foo() functions in xl_cmdimpl.c as some king of extensions of the
actual main function.

The alternative would be to always use, say, 0 and 1 in xl_cmdimpl.c,
and then convert them to EXIT_SUCCESS or EXIT_FAILURE in xl.c (for
return-s, of course, exit()-s need to use them no matter where they
are).

I'm fine with either, so, if you prefer the latter, I certainly can
arrange for doing things that way.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xl: log an error if libxl_cpupool_destroy() fails

[Ian Campbell]

On Fri, 2015-10-23 at 17:12 +0200, Dario Faggioli wrote:
> On Fri, 2015-10-23 at 15:09 +0100, Ian Campbell wrote:
> > On Fri, 2015-10-23 at 09:43 +0100, Wei Liu wrote:
> 
> > Looking at those links, I'm not sure that either you or myself was
> > thinking
> > of using EXIT_* as function return values, just that the eventual
> > process
> > exit would become one of those values instead of some negative
> > number.
> > 
> Exactly.
> 
> > Although the thread doesn't look like it is too clear if it is
> > talking
> > about return values from functions vs. process exit codes.
> > 
> Well, independently from the thread, I certainly meant that I think
> EXIT_* should be used as process exit status, not as internal
> functions' return value.
> 
> > I think what I would have been expecting is for the xl internal error
> > code
> > Well, same here. Except, given xl architecture, I was considering
> main_foo() functions in xl_cmdimpl.c as some king of extensions of the
> actual main function.

I had somehow convinced myself that these weren't being added in a
main_foo, I agree that main_foo should be treated somewhat like a regular
main().

Sorry for the noise.

> I'm fine with either, so, if you prefer the latter, I certainly can
> arrange for doing things that way.

It would be helpful to a) not combine this change with the logging change
and to b) include as part of the patch some sort of document comment in
some relevant xl-ish place explaining some of this stuff (i.e. that an xl
process should always return EXIT_FOO and that main_* can be treated like
main() as if they are returning a process exit status and not a function
return value).

I think it would also be useful to have xl's main() DTRT before starting to convert main_*. Currently it returns explicit 0 or 1 or the result of the main_*.

Ian.

Re: [PATCH] xl: log an error if libxl_cpupool_destroy() fails

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1880 bytes --]

On Fri, 2015-10-23 at 16:40 +0100, Ian Campbell wrote:
> On Fri, 2015-10-23 at 17:12 +0200, Dario Faggioli wrote:
> > On Fri, 2015-10-23 at 15:09 +0100, Ian Campbell wrote:

> > > I think what I would have been expecting is for the xl internal
> > > error
> > > code
> > Well, same here. Except, given xl architecture, I was considering
> > main_foo() functions in xl_cmdimpl.c as some king of extensions of
> > the
> > actual main function.
> 
> I had somehow convinced myself that these weren't being added in a
> main_foo, I agree that main_foo should be treated somewhat like a
> regular
> main().
> 
Ok, glad to see we're on the same page.

> Sorry for the noise.
> 
NP. :-)

> > I'm fine with either, so, if you prefer the latter, I certainly can
> > arrange for doing things that way.
> 
> It would be helpful to a) not combine this change with the logging 
> change
>
Sure, I'll resend the patch without changing that.

> b) include as part of the patch some sort of document comment in
> some relevant xl-ish place explaining some of this stuff (i.e. that
> an xl
> process should always return EXIT_FOO and that main_* can be treated
> like
> main() as if they are returning a process exit status and not a
> function
> return value).
> 
About this...

> I think it would also be useful to have xl's main() DTRT before
> starting to convert main_*. Currently it returns explicit 0 or 1 or
> the result of the main_*.
> 
... and this, I'll see if I can convince Harman to pick these up, as
part of her work on the subject. :-P :-P

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel