[PATCH for-4.5 0/2] xl/libxl: return and print partial config

[PATCH for-4.5 0/2] xl/libxl: return and print partial config

[Wei Liu]

This small series change the behavior of libxl_retrieve_domain_configuration,
to make it continue to retrieve information from xenstore even if JSON template
is not available.

This change of API behaviour is only internal. Conceptually speaking, any
non-zero return value means d_config is partially filled. The chanage just
makes it fill in more information (xenstore entries) than before (empty).
Caller can still expect zero on success and non-zero on error and act
accordingly.

"xl list -l" is now changed to print out the partial configuration, since it
needs to be consistent with the short output.

Wei.

Wei Liu (2):
  libxl: continue when encounter ERROR_JSON_CONFIG_EMPTY
  xl: print out partial configuration in long mode of list command

 tools/libxl/libxl.c      |    6 +++++-
 tools/libxl/xl_cmdimpl.c |    6 ++----
 2 files changed, 7 insertions(+), 5 deletions(-)

-- 
1.7.10.4

[PATCH for-4.5 1/2] libxl: continue when encounter ERROR_JSON_CONFIG_EMPTY

[Wei Liu]

Continue when libxl_retrieve_domain_configuration encounters
ERROR_JSON_CONFIG_EMPTY, as caller might be interested in the partial
configuration pulled from xenstore.  In this case
ERROR_JSON_CONFIG_EMPTY is used as return value as before, if no other
error happens along the way.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Zhigang Wang <zhigang.x.wang@oracle.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index f7961f6..f54e0ea 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -6521,6 +6521,7 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
     GC_INIT(ctx);
     int rc;
     libxl__domain_userdata_lock *lock = NULL;
+    bool json_empty = false;
 
     CTX_LOCK;
 
@@ -6531,11 +6532,12 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
     }
 
     rc = libxl__get_domain_configuration(gc, domid, d_config);
-    if (rc) {
+    if (rc && rc != ERROR_JSON_CONFIG_EMPTY) {
         LOG(ERROR, "fail to get domain configuration for domain %d", domid);
         rc = ERROR_FAIL;
         goto out;
     }
+    if (rc == ERROR_JSON_CONFIG_EMPTY) json_empty = true;
 
     /* Domain name */
     {
@@ -6692,6 +6694,8 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
 
 #undef MERGE
 
+    rc = json_empty ? ERROR_JSON_CONFIG_EMPTY : 0;
+
 out:
     if (lock) libxl__unlock_domain_userdata(lock);
     CTX_UNLOCK;
-- 
1.7.10.4

[PATCH for-4.5 2/2] xl: print out partial configuration in long mode of list command

[Wei Liu]

Unconditionally print out the partial configuration.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Zhigang Wang <zhigang.x.wang@oracle.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 3c9f146..396e06c 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3388,7 +3388,7 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
 {
     libxl_domain_config d_config;
 
-    int i, rc;
+    int i;
 
     yajl_gen hand = NULL;
     yajl_gen_status s;
@@ -3410,9 +3410,7 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
 
     for (i = 0; i < nb_domain; i++) {
         libxl_domain_config_init(&d_config);
-        rc = libxl_retrieve_domain_configuration(ctx, info[i].domid, &d_config);
-        if (rc)
-            continue;
+        libxl_retrieve_domain_configuration(ctx, info[i].domid, &d_config);
         if (default_output_format == OUTPUT_FORMAT_JSON)
             s = printf_info_one_json(hand, info[i].domid, &d_config);
         else
-- 
1.7.10.4

Re: [PATCH for-4.5 0/2] xl/libxl: return and print partial config

[Konrad Rzeszutek Wilk]

On Wed, Nov 12, 2014 at 05:04:23PM +0000, Wei Liu wrote:
> This small series change the behavior of libxl_retrieve_domain_configuration,
> to make it continue to retrieve information from xenstore even if JSON template
> is not available.
> 
> This change of API behaviour is only internal. Conceptually speaking, any
> non-zero return value means d_config is partially filled. The chanage just
> makes it fill in more information (xenstore entries) than before (empty).
> Caller can still expect zero on success and non-zero on error and act
> accordingly.

What are the work-arounds (if any) if this does not go in Xen 4.5?

> 
> "xl list -l" is now changed to print out the partial configuration, since it
> needs to be consistent with the short output.
> 
> Wei.
> 
> Wei Liu (2):
>   libxl: continue when encounter ERROR_JSON_CONFIG_EMPTY
>   xl: print out partial configuration in long mode of list command
> 
>  tools/libxl/libxl.c      |    6 +++++-
>  tools/libxl/xl_cmdimpl.c |    6 ++----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> -- 
> 1.7.10.4
> 

Re: [PATCH for-4.5 0/2] xl/libxl: return and print partial config

[Wei Liu]

On Wed, Nov 12, 2014 at 12:18:32PM -0500, Konrad Rzeszutek Wilk wrote:
> On Wed, Nov 12, 2014 at 05:04:23PM +0000, Wei Liu wrote:
> > This small series change the behavior of libxl_retrieve_domain_configuration,
> > to make it continue to retrieve information from xenstore even if JSON template
> > is not available.
> > 
> > This change of API behaviour is only internal. Conceptually speaking, any
> > non-zero return value means d_config is partially filled. The chanage just
> > makes it fill in more information (xenstore entries) than before (empty).
> > Caller can still expect zero on success and non-zero on error and act
> > accordingly.
> 
> What are the work-arounds (if any) if this does not go in Xen 4.5?
> 

The first patch to libxl doesn't change external visible behaviour, that
is, 0 on success, non-zero on error. So whether it goes in or not I
don't have very strong opinion. Keep in mind that this API is new so
this first patch wouldn't cause regression in any way.

For the inconsistency between short and long output, no workaround, but
it wouldn't be disastrous.

Wei.

> > 
> > "xl list -l" is now changed to print out the partial configuration, since it
> > needs to be consistent with the short output.
> > 
> > Wei.
> > 
> > Wei Liu (2):
> >   libxl: continue when encounter ERROR_JSON_CONFIG_EMPTY
> >   xl: print out partial configuration in long mode of list command
> > 
> >  tools/libxl/libxl.c      |    6 +++++-
> >  tools/libxl/xl_cmdimpl.c |    6 ++----
> >  2 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > -- 
> > 1.7.10.4
> > 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH for-4.5 0/2] xl/libxl: return and print partial config

[Zhigang Wang]

On 11/12/2014 12:04 PM, Wei Liu wrote:
> This small series change the behavior of libxl_retrieve_domain_configuration,
> to make it continue to retrieve information from xenstore even if JSON template
> is not available.
> 
> This change of API behaviour is only internal. Conceptually speaking, any
> non-zero return value means d_config is partially filled. The chanage just
> makes it fill in more information (xenstore entries) than before (empty).
> Caller can still expect zero on success and non-zero on error and act
> accordingly.
> 
> "xl list -l" is now changed to print out the partial configuration, since it
> needs to be consistent with the short output.
> 
> Wei.
> 
> Wei Liu (2):
>   libxl: continue when encounter ERROR_JSON_CONFIG_EMPTY
>   xl: print out partial configuration in long mode of list command
> 
>  tools/libxl/libxl.c      |    6 +++++-
>  tools/libxl/xl_cmdimpl.c |    6 ++----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 

It seem some other places may need fix too. Here is the test result:

# xl list -l
libxl: error: libxl.c:4844:libxl__get_memory_target: cannot get target memory info from /local/domain/1/memory/target
: No such file or directory
libxl: error: libxl.c:6582:libxl_retrieve_domain_configuration: fail to get memory target for domain 1
[
    {
        "domid": 0,
        "config": {
            "c_info": {
                "type": "pv",
                "name": "Domain-0"
            },
            "b_info": {
                "max_memkb": 876544,
                "target_memkb": 876543,
                "sched_params": {

                },
                "type.pv": {

                }
            }
        }
    },
    {
        "domid": 1,
        "config": {
            "c_info": {
                "name": "0004fb00000600008b1d33e2704548b7--incoming",
                "uuid": "0004fb00-0006-0000-8b1d-33e2704548b7"
            },
            "b_info": {
                "sched_params": {

                },
                "type.invalid": {

                }
            }
        }
    }
]

Thanks,

Zhigang

Re: [PATCH for-4.5 0/2] xl/libxl: return and print partial config

[Wei Liu]

On Wed, Nov 12, 2014 at 02:54:00PM -0500, Zhigang Wang wrote:
> On 11/12/2014 12:04 PM, Wei Liu wrote:
> > This small series change the behavior of libxl_retrieve_domain_configuration,
> > to make it continue to retrieve information from xenstore even if JSON template
> > is not available.
> > 
> > This change of API behaviour is only internal. Conceptually speaking, any
> > non-zero return value means d_config is partially filled. The chanage just
> > makes it fill in more information (xenstore entries) than before (empty).
> > Caller can still expect zero on success and non-zero on error and act
> > accordingly.
> > 
> > "xl list -l" is now changed to print out the partial configuration, since it
> > needs to be consistent with the short output.
> > 
> > Wei.
> > 
> > Wei Liu (2):
> >   libxl: continue when encounter ERROR_JSON_CONFIG_EMPTY
> >   xl: print out partial configuration in long mode of list command
> > 
> >  tools/libxl/libxl.c      |    6 +++++-
> >  tools/libxl/xl_cmdimpl.c |    6 ++----
> >  2 files changed, 7 insertions(+), 5 deletions(-)
> > 
> 
> It seem some other places may need fix too. Here is the test result:
> 
> # xl list -l
> libxl: error: libxl.c:4844:libxl__get_memory_target: cannot get target memory info from /local/domain/1/memory/target
> : No such file or directory
> libxl: error: libxl.c:6582:libxl_retrieve_domain_configuration: fail to get memory target for domain 1

Well, there's no such node in xenstore. There's nothing I can do about
it.

Wei.

Re: [PATCH for-4.5 0/2] xl/libxl: return and print partial config

[Konrad Rzeszutek Wilk]

On Wed, Nov 12, 2014 at 05:28:39PM +0000, Wei Liu wrote:
> On Wed, Nov 12, 2014 at 12:18:32PM -0500, Konrad Rzeszutek Wilk wrote:
> > On Wed, Nov 12, 2014 at 05:04:23PM +0000, Wei Liu wrote:
> > > This small series change the behavior of libxl_retrieve_domain_configuration,
> > > to make it continue to retrieve information from xenstore even if JSON template
> > > is not available.
> > > 
> > > This change of API behaviour is only internal. Conceptually speaking, any
> > > non-zero return value means d_config is partially filled. The chanage just
> > > makes it fill in more information (xenstore entries) than before (empty).
> > > Caller can still expect zero on success and non-zero on error and act
> > > accordingly.
> > 
> > What are the work-arounds (if any) if this does not go in Xen 4.5?
> > 
> 
> The first patch to libxl doesn't change external visible behaviour, that
> is, 0 on success, non-zero on error. So whether it goes in or not I
> don't have very strong opinion. Keep in mind that this API is new so
> this first patch wouldn't cause regression in any way.
> 
> For the inconsistency between short and long output, no workaround, but
> it wouldn't be disastrous.

They look both pretty innocuous so Release-Acked-by: Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com>

Thank you.
> 
> Wei.
> 
> > > 
> > > "xl list -l" is now changed to print out the partial configuration, since it
> > > needs to be consistent with the short output.
> > > 
> > > Wei.
> > > 
> > > Wei Liu (2):
> > >   libxl: continue when encounter ERROR_JSON_CONFIG_EMPTY
> > >   xl: print out partial configuration in long mode of list command
> > > 
> > >  tools/libxl/libxl.c      |    6 +++++-
> > >  tools/libxl/xl_cmdimpl.c |    6 ++----
> > >  2 files changed, 7 insertions(+), 5 deletions(-)
> > > 
> > > -- 
> > > 1.7.10.4
> > > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

Re: [PATCH for-4.5 2/2] xl: print out partial configuration in long mode of list command

[Ian Campbell]

On Wed, 2014-11-12 at 17:04 +0000, Wei Liu wrote:
> Unconditionally print out the partial configuration.

Can you provide an example of what such a configuration looks like?

> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Zhigang Wang <zhigang.x.wang@oracle.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 3c9f146..396e06c 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3388,7 +3388,7 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
>  {
>      libxl_domain_config d_config;
>  
> -    int i, rc;
> +    int i;
>  
>      yajl_gen hand = NULL;
>      yajl_gen_status s;
> @@ -3410,9 +3410,7 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
>  
>      for (i = 0; i < nb_domain; i++) {
>          libxl_domain_config_init(&d_config);
> -        rc = libxl_retrieve_domain_configuration(ctx, info[i].domid, &d_config);
> -        if (rc)
> -            continue;
> +        libxl_retrieve_domain_configuration(ctx, info[i].domid, &d_config);
>          if (default_output_format == OUTPUT_FORMAT_JSON)
>              s = printf_info_one_json(hand, info[i].domid, &d_config);
>          else

Re: [PATCH for-4.5 2/2] xl: print out partial configuration in long mode of list command

[Wei Liu]

On Fri, Nov 14, 2014 at 09:59:00AM +0000, Ian Campbell wrote:
> On Wed, 2014-11-12 at 17:04 +0000, Wei Liu wrote:
> > Unconditionally print out the partial configuration.
> 
> Can you provide an example of what such a configuration looks like?
> 

    {
        "domid": 2,
        "config": {
            "c_info": {
                "name": "s0-raw-vnuma",
                "uuid": "a8bed4ac-a0fe-4166-8eac-feeb007a2110"
            },
            "b_info": {
                "sched_params": {

                },
                "type.invalid": {

                }
            }
        }
    }

Libxl still complains because it tries to read some nodes that don't
exist, so xl will just print it out on stderr. This is the same
behaviour as before though.

Wei.

> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Cc: Zhigang Wang <zhigang.x.wang@oracle.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > ---
> >  tools/libxl/xl_cmdimpl.c |    6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 3c9f146..396e06c 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -3388,7 +3388,7 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
> >  {
> >      libxl_domain_config d_config;
> >  
> > -    int i, rc;
> > +    int i;
> >  
> >      yajl_gen hand = NULL;
> >      yajl_gen_status s;
> > @@ -3410,9 +3410,7 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
> >  
> >      for (i = 0; i < nb_domain; i++) {
> >          libxl_domain_config_init(&d_config);
> > -        rc = libxl_retrieve_domain_configuration(ctx, info[i].domid, &d_config);
> > -        if (rc)
> > -            continue;
> > +        libxl_retrieve_domain_configuration(ctx, info[i].domid, &d_config);
> >          if (default_output_format == OUTPUT_FORMAT_JSON)
> >              s = printf_info_one_json(hand, info[i].domid, &d_config);
> >          else
> 

Re: [PATCH for-4.5 2/2] xl: print out partial configuration in long mode of list command

[Ian Campbell]

On Fri, 2014-11-14 at 10:09 +0000, Wei Liu wrote:
> On Fri, Nov 14, 2014 at 09:59:00AM +0000, Ian Campbell wrote:
> > On Wed, 2014-11-12 at 17:04 +0000, Wei Liu wrote:
> > > Unconditionally print out the partial configuration.
> > 
> > Can you provide an example of what such a configuration looks like?
> > 
> 
>     {
>         "domid": 2,
>         "config": {
>             "c_info": {
>                 "name": "s0-raw-vnuma",
>                 "uuid": "a8bed4ac-a0fe-4166-8eac-feeb007a2110"
>             },
>             "b_info": {
>                 "sched_params": {
> 
>                 },
>                 "type.invalid": {
> 
>                 }
>             }
>         }
>     }

Great, thanks. I propose to insert this into the commit message as I
check it in (once I check who's acked it etc)

> Libxl still complains because it tries to read some nodes that don't
> exist, so xl will just print it out on stderr. This is the same
> behaviour as before though.

I think that's tolerable for 4.5 at least.

Ian.

Re: [PATCH for-4.5 1/2] libxl: continue when encounter ERROR_JSON_CONFIG_EMPTY

[Ian Campbell]

On Wed, 2014-11-12 at 17:04 +0000, Wei Liu wrote:
> Continue when libxl_retrieve_domain_configuration encounters
> ERROR_JSON_CONFIG_EMPTY, as caller might be interested in the partial
> configuration pulled from xenstore.  In this case
> ERROR_JSON_CONFIG_EMPTY is used as return value as before, if no other
> error happens along the way.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Zhigang Wang <zhigang.x.wang@oracle.com>

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

> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  tools/libxl/libxl.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index f7961f6..f54e0ea 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -6521,6 +6521,7 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
>      GC_INIT(ctx);
>      int rc;
>      libxl__domain_userdata_lock *lock = NULL;
> +    bool json_empty = false;
>  
>      CTX_LOCK;
>  
> @@ -6531,11 +6532,12 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
>      }
>  
>      rc = libxl__get_domain_configuration(gc, domid, d_config);
> -    if (rc) {
> +    if (rc && rc != ERROR_JSON_CONFIG_EMPTY) {
>          LOG(ERROR, "fail to get domain configuration for domain %d", domid);
>          rc = ERROR_FAIL;
>          goto out;
>      }
> +    if (rc == ERROR_JSON_CONFIG_EMPTY) json_empty = true;
>  
>      /* Domain name */
>      {
> @@ -6692,6 +6694,8 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
>  
>  #undef MERGE
>  
> +    rc = json_empty ? ERROR_JSON_CONFIG_EMPTY : 0;
> +
>  out:
>      if (lock) libxl__unlock_domain_userdata(lock);
>      CTX_UNLOCK;

Re: [PATCH for-4.5 2/2] xl: print out partial configuration in long mode of list command

[Ian Campbell]

On Wed, 2014-11-12 at 17:04 +0000, Wei Liu wrote:
> Unconditionally print out the partial configuration.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Zhigang Wang <zhigang.x.wang@oracle.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 3c9f146..396e06c 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3388,7 +3388,7 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
>  {
>      libxl_domain_config d_config;
>  
> -    int i, rc;
> +    int i;
>  
>      yajl_gen hand = NULL;
>      yajl_gen_status s;
> @@ -3410,9 +3410,7 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
>  
>      for (i = 0; i < nb_domain; i++) {
>          libxl_domain_config_init(&d_config);
> -        rc = libxl_retrieve_domain_configuration(ctx, info[i].domid, &d_config);
> -        if (rc)
> -            continue;
> +        libxl_retrieve_domain_configuration(ctx, info[i].domid, &d_config);

Should't this continue to skip cases where rc is not in {0,
ERROR_EMPTY_JSON}?

Ian.

Re: [PATCH for-4.5 1/2] libxl: continue when encounter ERROR_JSON_CONFIG_EMPTY

[Ian Campbell]

On Fri, 2014-11-14 at 10:52 +0000, Ian Campbell wrote:
> On Wed, 2014-11-12 at 17:04 +0000, Wei Liu wrote:
> > Continue when libxl_retrieve_domain_configuration encounters
> > ERROR_JSON_CONFIG_EMPTY, as caller might be interested in the partial
> > configuration pulled from xenstore.  In this case
> > ERROR_JSON_CONFIG_EMPTY is used as return value as before, if no other
> > error happens along the way.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Cc: Zhigang Wang <zhigang.x.wang@oracle.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

On second thoughts, I think this really needs an update to libxl.h to
describe the semantics of this function, i.e. to what extent the output
is valid for various error codes, especially ERROR_JSON_CONFIG_EMPTY.

> 
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > ---
> >  tools/libxl/libxl.c |    6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index f7961f6..f54e0ea 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -6521,6 +6521,7 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> >      GC_INIT(ctx);
> >      int rc;
> >      libxl__domain_userdata_lock *lock = NULL;
> > +    bool json_empty = false;
> >  
> >      CTX_LOCK;
> >  
> > @@ -6531,11 +6532,12 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> >      }
> >  
> >      rc = libxl__get_domain_configuration(gc, domid, d_config);
> > -    if (rc) {
> > +    if (rc && rc != ERROR_JSON_CONFIG_EMPTY) {
> >          LOG(ERROR, "fail to get domain configuration for domain %d", domid);
> >          rc = ERROR_FAIL;
> >          goto out;
> >      }
> > +    if (rc == ERROR_JSON_CONFIG_EMPTY) json_empty = true;
> >  
> >      /* Domain name */
> >      {
> > @@ -6692,6 +6694,8 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> >  
> >  #undef MERGE
> >  
> > +    rc = json_empty ? ERROR_JSON_CONFIG_EMPTY : 0;
> > +
> >  out:
> >      if (lock) libxl__unlock_domain_userdata(lock);
> >      CTX_UNLOCK;
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH for-4.5 2/2] xl: print out partial configuration in long mode of list command

[Wei Liu]

On Fri, Nov 14, 2014 at 10:53:45AM +0000, Ian Campbell wrote:
> On Wed, 2014-11-12 at 17:04 +0000, Wei Liu wrote:
> > Unconditionally print out the partial configuration.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Cc: Zhigang Wang <zhigang.x.wang@oracle.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > ---
> >  tools/libxl/xl_cmdimpl.c |    6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 3c9f146..396e06c 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -3388,7 +3388,7 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
> >  {
> >      libxl_domain_config d_config;
> >  
> > -    int i, rc;
> > +    int i;
> >  
> >      yajl_gen hand = NULL;
> >      yajl_gen_status s;
> > @@ -3410,9 +3410,7 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
> >  
> >      for (i = 0; i < nb_domain; i++) {
> >          libxl_domain_config_init(&d_config);
> > -        rc = libxl_retrieve_domain_configuration(ctx, info[i].domid, &d_config);
> > -        if (rc)
> > -            continue;
> > +        libxl_retrieve_domain_configuration(ctx, info[i].domid, &d_config);
> 
> Should't this continue to skip cases where rc is not in {0,
> ERROR_EMPTY_JSON}?
> 

Conceptually speaking, non-zero return value means this d_config is
partially filled. ERROR_JSON_CONFIG_EMPTY is as good / bad as any other
return values.

In this "xl list -l" case, We're interested in two things: to state the
fact that this domain exists and to print out the config.

Domain's existence is confirmed at this point, and we should print out
that partially filled d_config. How much information is filled in is
another question though.  Only printing out d_config when {0,
ERROR_JSON_CONFIG_EMPTY} makes "xl list -l" inconsistent with "xl list".

Wei.

> Ian.

Re: [PATCH for-4.5 1/2] libxl: continue when encounter ERROR_JSON_CONFIG_EMPTY

[Wei Liu]

On Fri, Nov 14, 2014 at 10:55:35AM +0000, Ian Campbell wrote:
> On Fri, 2014-11-14 at 10:52 +0000, Ian Campbell wrote:
> > On Wed, 2014-11-12 at 17:04 +0000, Wei Liu wrote:
> > > Continue when libxl_retrieve_domain_configuration encounters
> > > ERROR_JSON_CONFIG_EMPTY, as caller might be interested in the partial
> > > configuration pulled from xenstore.  In this case
> > > ERROR_JSON_CONFIG_EMPTY is used as return value as before, if no other
> > > error happens along the way.
> > > 
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > Cc: Zhigang Wang <zhigang.x.wang@oracle.com>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> On second thoughts, I think this really needs an update to libxl.h to
> describe the semantics of this function, i.e. to what extent the output
> is valid for various error codes, especially ERROR_JSON_CONFIG_EMPTY.
> 

Any non-zero return code means the output is invalid (as in "Is this
output valid to rebuild a domain?").

Wei.

Re: [PATCH for-4.5 1/2] libxl: continue when encounter ERROR_JSON_CONFIG_EMPTY

[Ian Campbell]

On Fri, 2014-11-14 at 11:10 +0000, Wei Liu wrote:
> On Fri, Nov 14, 2014 at 10:55:35AM +0000, Ian Campbell wrote:
> > On Fri, 2014-11-14 at 10:52 +0000, Ian Campbell wrote:
> > > On Wed, 2014-11-12 at 17:04 +0000, Wei Liu wrote:
> > > > Continue when libxl_retrieve_domain_configuration encounters
> > > > ERROR_JSON_CONFIG_EMPTY, as caller might be interested in the partial
> > > > configuration pulled from xenstore.  In this case
> > > > ERROR_JSON_CONFIG_EMPTY is used as return value as before, if no other
> > > > error happens along the way.
> > > > 
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > Cc: Zhigang Wang <zhigang.x.wang@oracle.com>
> > > 
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > On second thoughts, I think this really needs an update to libxl.h to
> > describe the semantics of this function, i.e. to what extent the output
> > is valid for various error codes, especially ERROR_JSON_CONFIG_EMPTY.
> > 
> 
> Any non-zero return code means the output is invalid (as in "Is this
> output valid to rebuild a domain?").

But your second patch prints it as if it is at least somewhat
meaningful, if not entirely valid. According to what you just said it
shouldn't do so.

You effectively have three return states now: Fully valid, domain exists
but it's configuration is unsure or somehow incomplete (~= JSON EMPTY),
some sort of error occurred.

It might even be a good idea to have some new externally visible error
code for the middle state, since JSON_EMPTY may not be the only reason
for being in that state (at least in theory).

Anyway, since this is all more subtle than the existing 0 is good,
non-zero is completely bad it should be written down.

Ian.

Re: [PATCH for-4.5 1/2] libxl: continue when encounter ERROR_JSON_CONFIG_EMPTY

[Wei Liu]

On Fri, Nov 14, 2014 at 11:19:56AM +0000, Ian Campbell wrote:
> On Fri, 2014-11-14 at 11:10 +0000, Wei Liu wrote:
> > On Fri, Nov 14, 2014 at 10:55:35AM +0000, Ian Campbell wrote:
> > > On Fri, 2014-11-14 at 10:52 +0000, Ian Campbell wrote:
> > > > On Wed, 2014-11-12 at 17:04 +0000, Wei Liu wrote:
> > > > > Continue when libxl_retrieve_domain_configuration encounters
> > > > > ERROR_JSON_CONFIG_EMPTY, as caller might be interested in the partial
> > > > > configuration pulled from xenstore.  In this case
> > > > > ERROR_JSON_CONFIG_EMPTY is used as return value as before, if no other
> > > > > error happens along the way.
> > > > > 
> > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > > Cc: Zhigang Wang <zhigang.x.wang@oracle.com>
> > > > 
> > > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > On second thoughts, I think this really needs an update to libxl.h to
> > > describe the semantics of this function, i.e. to what extent the output
> > > is valid for various error codes, especially ERROR_JSON_CONFIG_EMPTY.
> > > 
> > 
> > Any non-zero return code means the output is invalid (as in "Is this
> > output valid to rebuild a domain?").
> 
> But your second patch prints it as if it is at least somewhat
> meaningful, if not entirely valid. According to what you just said it
> shouldn't do so.
> 
> You effectively have three return states now: Fully valid, domain exists
> but it's configuration is unsure or somehow incomplete (~= JSON EMPTY),
> some sort of error occurred.
> 
> It might even be a good idea to have some new externally visible error
> code for the middle state, since JSON_EMPTY may not be the only reason
> for being in that state (at least in theory).
> 
> Anyway, since this is all more subtle than the existing 0 is good,
> non-zero is completely bad it should be written down.
> 

I'm not very keen on rushing to get ERROR_JSON_CONFIG_EMPTY special
meaning at this point of a release. I think we can deal with this in
next release.

Wei.

> Ian.

Re: [PATCH for-4.5 2/2] xl: print out partial configuration in long mode of list command

[Zhigang Wang]

On 11/14/2014 05:22 AM, Ian Campbell wrote:
> On Fri, 2014-11-14 at 10:09 +0000, Wei Liu wrote:
>> On Fri, Nov 14, 2014 at 09:59:00AM +0000, Ian Campbell wrote:
>>> On Wed, 2014-11-12 at 17:04 +0000, Wei Liu wrote:
>>>> Unconditionally print out the partial configuration.
>>>
>>> Can you provide an example of what such a configuration looks like?
>>>
>>
>>     {
>>         "domid": 2,
>>         "config": {
>>             "c_info": {
>>                 "name": "s0-raw-vnuma",
>>                 "uuid": "a8bed4ac-a0fe-4166-8eac-feeb007a2110"
>>             },
>>             "b_info": {
>>                 "sched_params": {
>>
>>                 },
>>                 "type.invalid": {
>>
>>                 }
>>             }
>>         }
>>     }
> 
> Great, thanks. I propose to insert this into the commit message as I
> check it in (once I check who's acked it etc)
> 
>> Libxl still complains because it tries to read some nodes that don't
>> exist, so xl will just print it out on stderr. This is the same
>> behaviour as before though.
> 
> I think that's tolerable for 4.5 at least.

I also think this is OK for 4.5.

Konrad: can pickup this for next rc?

Thanks,

Zhigang

Re: [PATCH for-4.5 2/2] xl: print out partial configuration in long mode of list command

[Konrad Rzeszutek Wilk]

On Fri, Nov 21, 2014 at 01:04:42PM -0500, Zhigang Wang wrote:
> On 11/14/2014 05:22 AM, Ian Campbell wrote:
> > On Fri, 2014-11-14 at 10:09 +0000, Wei Liu wrote:
> >> On Fri, Nov 14, 2014 at 09:59:00AM +0000, Ian Campbell wrote:
> >>> On Wed, 2014-11-12 at 17:04 +0000, Wei Liu wrote:
> >>>> Unconditionally print out the partial configuration.
> >>>
> >>> Can you provide an example of what such a configuration looks like?
> >>>
> >>
> >>     {
> >>         "domid": 2,
> >>         "config": {
> >>             "c_info": {
> >>                 "name": "s0-raw-vnuma",
> >>                 "uuid": "a8bed4ac-a0fe-4166-8eac-feeb007a2110"
> >>             },
> >>             "b_info": {
> >>                 "sched_params": {
> >>
> >>                 },
> >>                 "type.invalid": {
> >>
> >>                 }
> >>             }
> >>         }
> >>     }
> > 
> > Great, thanks. I propose to insert this into the commit message as I
> > check it in (once I check who's acked it etc)
> > 
> >> Libxl still complains because it tries to read some nodes that don't
> >> exist, so xl will just print it out on stderr. This is the same
> >> behaviour as before though.
> > 
> > I think that's tolerable for 4.5 at least.
> 
> I also think this is OK for 4.5.
> 
> Konrad: can pickup this for next rc?

Please see http://lists.xen.org/archives/html/xen-devel/2014-11/msg01238.html
> 
> Thanks,
> 
> Zhigang
> 

Re: [PATCH for-4.5 2/2] xl: print out partial configuration in long mode of list command

[Wei Liu]

On Fri, Nov 21, 2014 at 01:09:07PM -0500, Konrad Rzeszutek Wilk wrote:
> On Fri, Nov 21, 2014 at 01:04:42PM -0500, Zhigang Wang wrote:
> > On 11/14/2014 05:22 AM, Ian Campbell wrote:
> > > On Fri, 2014-11-14 at 10:09 +0000, Wei Liu wrote:
> > >> On Fri, Nov 14, 2014 at 09:59:00AM +0000, Ian Campbell wrote:
> > >>> On Wed, 2014-11-12 at 17:04 +0000, Wei Liu wrote:
> > >>>> Unconditionally print out the partial configuration.
> > >>>
> > >>> Can you provide an example of what such a configuration looks like?
> > >>>
> > >>
> > >>     {
> > >>         "domid": 2,
> > >>         "config": {
> > >>             "c_info": {
> > >>                 "name": "s0-raw-vnuma",
> > >>                 "uuid": "a8bed4ac-a0fe-4166-8eac-feeb007a2110"
> > >>             },
> > >>             "b_info": {
> > >>                 "sched_params": {
> > >>
> > >>                 },
> > >>                 "type.invalid": {
> > >>
> > >>                 }
> > >>             }
> > >>         }
> > >>     }
> > > 
> > > Great, thanks. I propose to insert this into the commit message as I
> > > check it in (once I check who's acked it etc)
> > > 
> > >> Libxl still complains because it tries to read some nodes that don't
> > >> exist, so xl will just print it out on stderr. This is the same
> > >> behaviour as before though.
> > > 
> > > I think that's tolerable for 4.5 at least.
> > 
> > I also think this is OK for 4.5.
> > 
> > Konrad: can pickup this for next rc?
> 
> Please see http://lists.xen.org/archives/html/xen-devel/2014-11/msg01238.html

Unfortunately I myself want to retract this series because I don't want
to give special consideration for ERROR_JSON_CONFIG_EMPTY.

I think all you need is to have such skeleton, right? I can do it in
xl...

Wei.

> > 
> > Thanks,
> > 
> > Zhigang
> > 

Re: [PATCH for-4.5 2/2] xl: print out partial configuration in long mode of list command

[Zhigang Wang]

On 11/21/2014 01:14 PM, Wei Liu wrote:
> On Fri, Nov 21, 2014 at 01:09:07PM -0500, Konrad Rzeszutek Wilk wrote:
>> On Fri, Nov 21, 2014 at 01:04:42PM -0500, Zhigang Wang wrote:
>>> On 11/14/2014 05:22 AM, Ian Campbell wrote:
>>>> On Fri, 2014-11-14 at 10:09 +0000, Wei Liu wrote:
>>>>> On Fri, Nov 14, 2014 at 09:59:00AM +0000, Ian Campbell wrote:
>>>>>> On Wed, 2014-11-12 at 17:04 +0000, Wei Liu wrote:
>>>>>>> Unconditionally print out the partial configuration.
>>>>>>
>>>>>> Can you provide an example of what such a configuration looks like?
>>>>>>
>>>>>
>>>>>     {
>>>>>         "domid": 2,
>>>>>         "config": {
>>>>>             "c_info": {
>>>>>                 "name": "s0-raw-vnuma",
>>>>>                 "uuid": "a8bed4ac-a0fe-4166-8eac-feeb007a2110"
>>>>>             },
>>>>>             "b_info": {
>>>>>                 "sched_params": {
>>>>>
>>>>>                 },
>>>>>                 "type.invalid": {
>>>>>
>>>>>                 }
>>>>>             }
>>>>>         }
>>>>>     }
>>>>
>>>> Great, thanks. I propose to insert this into the commit message as I
>>>> check it in (once I check who's acked it etc)
>>>>
>>>>> Libxl still complains because it tries to read some nodes that don't
>>>>> exist, so xl will just print it out on stderr. This is the same
>>>>> behaviour as before though.
>>>>
>>>> I think that's tolerable for 4.5 at least.
>>>
>>> I also think this is OK for 4.5.
>>>
>>> Konrad: can pickup this for next rc?
>>
>> Please see http://lists.xen.org/archives/html/xen-devel/2014-11/msg01238.html
> 
> Unfortunately I myself want to retract this series because I don't want
> to give special consideration for ERROR_JSON_CONFIG_EMPTY.
> 
> I think all you need is to have such skeleton, right? I can do it in
> xl...

Yes. I need a skeleton in `xl list -l` to indicate the domain is running.

If we can have the info as in `xl list`, that will be great:

   domid, name, uuid, avail_vcpus, target_memkb

   # xl list
    Name                                          ID   Mem VCPUs      State   Time(s)
    Domain-0                                       0   856     4     r-----    2758.9
    0004fb00000600003f327a843a5f2b72--incoming     7   131     1     --p---       0.0

Thanks,

Zhigang