[PATCH] xl: Change output from xl -N create to be more useful

[PATCH] xl: Change output from xl -N create to be more useful

[Ian Jackson]

Currently, xl -N create produces:

    {
        "domid": null,
        "config": {
            "c_info": {
                "type": "pv",
     [etc]
    }

The domid is always NULL (as the domain has not been created at this
stage).

This is annoying if you want to take this output and use it for some
actually useful purpose like domain creation: either it needs to be
massaged, or the the consuming tool needs to be taught to look inside
the json object for the `config' element (which IMO makes no sense as
an interface).

We would like to be able to pass libxl json configs around sensibly.
In the future maybe xl will grow an option to create a domain from a
json config, and this is currently something I want to be able to have
a test tool do.

Note that this change is NOT BACKWARDS COMPATIBLE.  But it would only
adversely affects anyone who uses `xl -N create' and then saves and
processes the JSON.  (The output from xl list et al is not changed; it
normally needs the domid.)  Such a user should probably have already
have complained about the infelicitous output.  If they haven't it
would be simple enough for them to bookend the output so as to provide
compatible output.

If this backward compatibility problem is considered a blocker for
this patch, then I will respin, with one of the following two
workarounds:
  - A new option to force sane output
  - Generate output which contains the domain config twice,
    once directly in the main struct, and a copy in "config"

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Euan Harris <euan.harris@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c858068..9e9ee5e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2687,9 +2687,20 @@ static uint32_t create_domain(struct domain_create *dom_info)
         }
     }
 
-    if (debug || dom_info->dryrun)
-        printf_info(default_output_format, -1, &d_config,
-                    debug ? stderr : stdout);
+    if (debug || dom_info->dryrun) {
+        FILE *cfg_print_fh = debug ? stderr : stdout;
+        if (default_output_format == OUTPUT_FORMAT_SXP) {
+            printf_info_sexp(-1, &d_config, cfg_print_fh);
+        } else {
+            char *json = libxl_domain_config_to_json(ctx, &d_config);
+            fputs(json, stdout);
+            free(json);
+            if (ferror(stdout) || fflush(stdout)) {
+                perror("stdout"); exit(-1);
+            }
+        }
+    }
+
 
     ret = 0;
     if (dom_info->dryrun)
-- 
1.7.10.4

Re: [PATCH] xl: Change output from xl -N create to be more useful

[Ian Jackson]

Ian Jackson writes ("[PATCH] xl: Change output from xl -N create to be more useful"):
> Currently, xl -N create produces:

With this change, and these commits to Euan's xl-test setup

  http://xenbits.xen.org/gitweb/?p=people/iwj/ring3-xl-test.git;a=summary
  git://xenbits.xen.org/people/iwj/ring3-xl-test.git
     #json-domain-config

I am able to run Euan's domain create test case on a domain generated
by osstest, and all the tests still pass.  This means that the test
suite (or at least, this test program) is useable without having to
download the provided resources/ things.

I ran
  xl -N create --quiet /etc/xen/debian.guest.osstest.cfg >/root/cfg
and then set
  XLTEST_CFG_test_domain_create_new=/root/cfg
in the test program's environment.

Ian.

Re: [PATCH] xl: Change output from xl -N create to be more useful

[Wei Liu]

On Fri, Jun 26, 2015 at 03:29:15PM +0100, Ian Jackson wrote:
> Currently, xl -N create produces:
> 
>     {
>         "domid": null,
>         "config": {
>             "c_info": {
>                 "type": "pv",
>      [etc]
>     }
> 
> The domid is always NULL (as the domain has not been created at this
> stage).
> 
> This is annoying if you want to take this output and use it for some
> actually useful purpose like domain creation: either it needs to be
> massaged, or the the consuming tool needs to be taught to look inside
> the json object for the `config' element (which IMO makes no sense as
> an interface).
> 
> We would like to be able to pass libxl json configs around sensibly.
> In the future maybe xl will grow an option to create a domain from a
> json config, and this is currently something I want to be able to have
> a test tool do.
> 
> Note that this change is NOT BACKWARDS COMPATIBLE.  But it would only
> adversely affects anyone who uses `xl -N create' and then saves and
> processes the JSON.  (The output from xl list et al is not changed; it
> normally needs the domid.)  Such a user should probably have already
> have complained about the infelicitous output.  If they haven't it
> would be simple enough for them to bookend the output so as to provide
> compatible output.
> 
> If this backward compatibility problem is considered a blocker for
> this patch, then I will respin, with one of the following two
> workarounds:
>   - A new option to force sane output
>   - Generate output which contains the domain config twice,
>     once directly in the main struct, and a copy in "config"

I don't think keeping a broken interface for the sake of backward
compatibility is worth it.

> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Euan Harris <euan.harris@citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index c858068..9e9ee5e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2687,9 +2687,20 @@ static uint32_t create_domain(struct domain_create *dom_info)
>          }
>      }
>  
> -    if (debug || dom_info->dryrun)
> -        printf_info(default_output_format, -1, &d_config,
> -                    debug ? stderr : stdout);
> +    if (debug || dom_info->dryrun) {
> +        FILE *cfg_print_fh = debug ? stderr : stdout;
> +        if (default_output_format == OUTPUT_FORMAT_SXP) {
> +            printf_info_sexp(-1, &d_config, cfg_print_fh);
> +        } else {
> +            char *json = libxl_domain_config_to_json(ctx, &d_config);
> +            fputs(json, stdout);
> +            free(json);
> +            if (ferror(stdout) || fflush(stdout)) {
> +                perror("stdout"); exit(-1);
> +            }
> +        }
> +    }
> +
>  

Actually you may want to update main_config_update, which also prints
out domain configuration. Then remove the will-be-defunct
printf_info{,_one_json}.

Wei.

>      ret = 0;
>      if (dom_info->dryrun)
> -- 
> 1.7.10.4

Re: [PATCH] xl: Change output from xl -N create to be more useful

[Ian Jackson]

Wei Liu writes ("Re: [PATCH] xl: Change output from xl -N create to be more useful"):
> On Fri, Jun 26, 2015 at 03:29:15PM +0100, Ian Jackson wrote:
...
> > Note that this change is NOT BACKWARDS COMPATIBLE.  But it would only
> > adversely affects anyone who uses `xl -N create' and then saves and
> > processes the JSON.  (The output from xl list et al is not changed; it
> > normally needs the domid.)  Such a user should probably have already
> > have complained about the infelicitous output.  If they haven't it
> > would be simple enough for them to bookend the output so as to provide
> > compatible output.
> > 
> > If this backward compatibility problem is considered a blocker for
> > this patch, then I will respin, with one of the following two
> > workarounds:
> >   - A new option to force sane output
> >   - Generate output which contains the domain config twice,
> >     once directly in the main struct, and a copy in "config"
> 
> I don't think keeping a broken interface for the sake of backward
> compatibility is worth it.

The interface isn't unuseable.  You just have to use jq(1) or
something to transform the output.

AFAIAA we have no in-tree consumers of libxl json domain configs and
further I'm not aware of any out-of-tree consumers apart from the one
I just introduced into the xs-ring3 ao abort test suite.

But, thanks for the favourable opinion :-).

> Actually you may want to update main_config_update, which also prints
> out domain configuration. Then remove the will-be-defunct
> printf_info{,_one_json}.

I'll look into doing this, thanks.

Ian.

Re: [PATCH] xl: Change output from xl -N create to be more useful

[Ian Campbell]

On Fri, 2015-06-26 at 16:30 +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH] xl: Change output from xl -N create to be more useful"):
> > On Fri, Jun 26, 2015 at 03:29:15PM +0100, Ian Jackson wrote:
> ...
> > > Note that this change is NOT BACKWARDS COMPATIBLE.  But it would only
> > > adversely affects anyone who uses `xl -N create' and then saves and
> > > processes the JSON.  (The output from xl list et al is not changed; it
> > > normally needs the domid.)  Such a user should probably have already
> > > have complained about the infelicitous output.  If they haven't it
> > > would be simple enough for them to bookend the output so as to provide
> > > compatible output.
> > > 
> > > If this backward compatibility problem is considered a blocker for
> > > this patch, then I will respin, with one of the following two
> > > workarounds:
> > >   - A new option to force sane output
> > >   - Generate output which contains the domain config twice,
> > >     once directly in the main struct, and a copy in "config"
> > 
> > I don't think keeping a broken interface for the sake of backward
> > compatibility is worth it.
> 
> The interface isn't unuseable.  You just have to use jq(1) or
> something to transform the output.
> 
> AFAIAA we have no in-tree consumers of libxl json domain configs and
> further I'm not aware of any out-of-tree consumers apart from the one
> I just introduced into the xs-ring3 ao abort test suite.
> 
> But, thanks for the favourable opinion :-).

I think we should just risk the change and if anyone notices and cares
we could consider retrofitting OUTPUT_FORMAT_JSON_XEN45 to xl. I think
it's unlikely anyone will notice.

Ian.

Re: [PATCH] xl: Change output from xl -N create to be more useful

[Ian Jackson]

Ian Jackson writes ("[PATCH] xl: Change output from xl -N create to be more useful"):
> Note that this change is NOT BACKWARDS COMPATIBLE.  But it would only
> adversely affects anyone who uses `xl -N create' and then saves and
> processes the JSON.  (The output from xl list et al is not changed; it
> normally needs the domid.)  Such a user should probably have already
> have complained about the infelicitous output.  If they haven't it
> would be simple enough for them to bookend the output so as to provide
> compatible output.

This was discussed and everyone was content with this implication of
the patch as-is.

Would anyone care to give it a formal ack ?

Thanks,
Ian.

Re: [PATCH] xl: Change output from xl -N create to be more useful

[Ian Campbell]

On Fri, 2015-06-26 at 15:29 +0100, Ian Jackson wrote:
> Currently, xl -N create produces:
> 
>     {
>         "domid": null,
>         "config": {
>             "c_info": {
>                 "type": "pv",
>      [etc]
>     }
> 
> The domid is always NULL (as the domain has not been created at this
> stage).
> 
> This is annoying if you want to take this output and use it for some
> actually useful purpose like domain creation: either it needs to be
> massaged, or the the consuming tool needs to be taught to look inside
> the json object for the `config' element (which IMO makes no sense as
> an interface).
> 
> We would like to be able to pass libxl json configs around sensibly.
> In the future maybe xl will grow an option to create a domain from a
> json config, and this is currently something I want to be able to have
> a test tool do.
> 
> Note that this change is NOT BACKWARDS COMPATIBLE.  But it would only
> adversely affects anyone who uses `xl -N create' and then saves and
> processes the JSON.  (The output from xl list et al is not changed; it
> normally needs the domid.)  Such a user should probably have already
> have complained about the infelicitous output.  If they haven't it
> would be simple enough for them to bookend the output so as to provide
> compatible output.
> 
> If this backward compatibility problem is considered a blocker for
> this patch, then I will respin, with one of the following two
> workarounds:
>   - A new option to force sane output
>   - Generate output which contains the domain config twice,
>     once directly in the main struct, and a copy in "config"
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Euan Harris <euan.harris@citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index c858068..9e9ee5e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2687,9 +2687,20 @@ static uint32_t create_domain(struct domain_create *dom_info)
>          }
>      }
>  
> -    if (debug || dom_info->dryrun)
> -        printf_info(default_output_format, -1, &d_config,
> -                    debug ? stderr : stdout);
> +    if (debug || dom_info->dryrun) {
> +        FILE *cfg_print_fh = debug ? stderr : stdout;

Did you mean to use this instead of the hardcoded stdout below?

Otherwise the debug output's location differs depending on the format,
which seems unexpected.

> +        if (default_output_format == OUTPUT_FORMAT_SXP) {
> +            printf_info_sexp(-1, &d_config, cfg_print_fh);
> +        } else {
> +            char *json = libxl_domain_config_to_json(ctx, &d_config);
> +            fputs(json, stdout);
> +            free(json);
> +            if (ferror(stdout) || fflush(stdout)) {
> +                perror("stdout"); exit(-1);
> +            }
> +        }
> +    }
> +
>  
>      ret = 0;
>      if (dom_info->dryrun)

Re: [PATCH] xl: Change output from xl -N create to be more useful

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] xl: Change output from xl -N create to be more useful"):
> On Fri, 2015-06-26 at 15:29 +0100, Ian Jackson wrote:
...
> > -    if (debug || dom_info->dryrun)
> > -        printf_info(default_output_format, -1, &d_config,
> > -                    debug ? stderr : stdout);
> > +    if (debug || dom_info->dryrun) {
> > +        FILE *cfg_print_fh = debug ? stderr : stdout;
> 
> Did you mean to use this instead of the hardcoded stdout below?
> 
> Otherwise the debug output's location differs depending on the format,
> which seems unexpected.

This is indeed wrong.  Well spotted.

Ian.

[PATCH 1/3] xl: Break out flush_stream

[Ian Jackson]

We are going to want to reuse this.  Adjust the code slightly to
detect right away call sites that pass something other than stdout or
stderr.

No resulting functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v2: New patch in this version of the mini-series
---
 tools/libxl/xl_cmdimpl.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c858068..ee55786 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -389,6 +389,20 @@ static yajl_gen_status printf_info_one_json(yajl_gen hand, int domid,
 out:
     return s;
 }
+
+static void flush_stream(FILE *fh)
+{
+    const char *fh_name =
+        fh == stdout ? "stdout" :
+        fh == stderr ? "stderr" :
+        (abort(), (const char*)0);
+
+    if (ferror(fh) || fflush(fh)) {
+        perror(fh_name);
+        exit(-1);
+    }
+}
+
 static void printf_info(enum output_format output_format,
                         int domid,
                         libxl_domain_config *d_config, FILE *fh)
@@ -424,13 +438,7 @@ out:
         fprintf(stderr,
                 "unable to format domain config as JSON (YAJL:%d)\n", s);
 
-    if (ferror(fh) || fflush(fh)) {
-        if (fh == stdout)
-            perror("stdout");
-        else
-            perror("stderr");
-        exit(-1);
-    }
+    flush_stream(fh);
 }
 
 static int do_daemonize(char *name)
-- 
1.7.10.4

[PATCH 2/3] xl: Change output from xl -N create to be more useful

[Ian Jackson]

Currently, xl -N create produces:

    {
        "domid": null,
        "config": {
            "c_info": {
                "type": "pv",
     [etc]
    }

The domid is always NULL (as the domain has not been created at this
stage).

This is annoying if you want to take this output and use it for some
actually useful purpose like domain creation: either it needs to be
massaged, or the the consuming tool needs to be taught to look inside
the json object for the `config' element (which IMO makes no sense as
an interface).

We would like to be able to pass libxl json configs around sensibly.
In the future maybe xl will grow an option to create a domain from a
json config, and this is currently something I want to be able to have
a test tool do.

Note that this change is NOT BACKWARDS COMPATIBLE.  But it would only
adversely affects anyone who uses `xl -N create' and then saves and
processes the JSON.  (The output from xl list et al is not changed; it
normally needs the domid.)  Such a user should probably have already
have complained about the infelicitous output.  If they haven't it
would be simple enough for them to bookend the output so as to provide
compatible output.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Euan Harris <euan.harris@citrix.com>
---
v2: Print json output to correct filehandle
    (Using newly introduced flush_stream.)
---
 tools/libxl/xl_cmdimpl.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ee55786..50247de 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2695,9 +2695,18 @@ static uint32_t create_domain(struct domain_create *dom_info)
         }
     }
 
-    if (debug || dom_info->dryrun)
-        printf_info(default_output_format, -1, &d_config,
-                    debug ? stderr : stdout);
+    if (debug || dom_info->dryrun) {
+        FILE *cfg_print_fh = debug ? stderr : stdout;
+        if (default_output_format == OUTPUT_FORMAT_SXP) {
+            printf_info_sexp(-1, &d_config, cfg_print_fh);
+        } else {
+            char *json = libxl_domain_config_to_json(ctx, &d_config);
+            fputs(json, cfg_print_fh);
+            free(json);
+            flush_stream(cfg_print_fh);
+        }
+    }
+
 
     ret = 0;
     if (dom_info->dryrun)
-- 
1.7.10.4

Re: [PATCH 2/3] xl: Change output from xl -N create to be more useful [and 1 more messages]

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/3] xl: Break out flush_stream"):
> All three patched: Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks, pushed.

Ian.

[PATCH 3/3] xl: xl -N create -d sends json output to stdout, not stderr

[Ian Jackson]

                            domain config output goes to
                              before       after

  xl    create                nowhere      nowhere
  xl    create -d             stderr       stderr

  xl -N create                stdout       stdout
  xl -N create -d             stderr       stdout

It is not sensible that adding -d would cause different output on
stdout.  And that -N would produce less debug output is hardly
surprising in general and not really a problem in this case.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v2: New patch in this version of the mini-series.
---
 tools/libxl/xl_cmdimpl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 50247de..1be3f8b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2696,7 +2696,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
     }
 
     if (debug || dom_info->dryrun) {
-        FILE *cfg_print_fh = debug ? stderr : stdout;
+        FILE *cfg_print_fh = (debug && !dom_info->dryrun) ? stderr : stdout;
         if (default_output_format == OUTPUT_FORMAT_SXP) {
             printf_info_sexp(-1, &d_config, cfg_print_fh);
         } else {
-- 
1.7.10.4

Re: [PATCH 1/3] xl: Break out flush_stream

[Ian Campbell]

On Fri, 2015-07-03 at 12:48 +0100, Ian Jackson wrote:

All three patched: Acked-by: Ian Campbell <ian.campbell@citrix.com>