[PATCH RFC] improve the error message in "xl list --long"

[PATCH RFC] improve the error message in "xl list --long"

[Bamvor Jian Zhang]

with this patch, xl will raise a proper error when execute "xl list --long"
for existed domain but no domain configuration found. it would encounter
if other toolstack(like libvirt) create the vm, xl could saw the domain
name but could not get the domain configuration.

there is a commit a76377f1 check the return value of
libxl_read_file_contents in libxl_userdata_retrieve. but skip the ENOENT.
it is almost three years before. i do not know the senario about that
commit. it seems that it is not safe to simplely remove the ENOENT.

Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com>
---
 tools/libxl/libxl_dom.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 356f920..8daf51a 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1655,6 +1655,12 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
         goto out;
     }
 
+    if (access(filename, F_OK) != 0) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Unknown domain configuration. Is domain id '%u' owned by another libxl toolstack?", domid);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
     e = libxl_read_file_contents(ctx, filename, data_r ? &data : 0, &datalen);
     if (e && errno != ENOENT) {
         rc = ERROR_FAIL;
-- 
1.8.1.4

Re: [PATCH RFC] improve the error message in "xl list --long"

[Ian Jackson]

Bamvor Jian Zhang writes ("[PATCH RFC] improve the error message in "xl list --long""):
> there is a commit a76377f1 check the return value of
> libxl_read_file_contents in libxl_userdata_retrieve. but skip the ENOENT.
> it is almost three years before. i do not know the senario about that
> commit. it seems that it is not safe to simplely remove the ENOENT.

ENOENT means that there is no userdata associated with the relevant
domain and key.  This is not an error condition.

And it is deliberately possible to manipulate other toolstacks'
domains from xl.  You get to keep all the pieces.  If you want to make
this fail, then a safety catch might be OK but it should be in xl not
libxl and I think the manipulation should be allowed by default.

Ian.

Re: [PATCH RFC] improve the error message in "xl list --long"

[Bamvor Jian Zhang]

Hi, Ian


thanks your reply.
> Bamvor Jian Zhang writes ("[PATCH RFC] improve the error message in "xl list
>         --long""):
> > there is a commit a76377f1 check the return value of
> > libxl_read_file_contents in libxl_userdata_retrieve. but skip the ENOENT.
> > it is almost three years before. i do not know the senario about that
> > commit. it seems that it is not safe to simplely remove the ENOENT.
>
> ENOENT means that there is no userdata associated with the relevant
> domain and key.  This is not an error condition.
>
> And it is deliberately possible to manipulate other toolstacks'
> domains from xl.  You get to keep all the pieces.
considering this case, the user use virt-manager for management the virtual
machine, he would not aware of xm, xl or virsh is used by virt-manager and
vm-install. and it is safe to use the xm with virt-manager because the vm

info is only stored in xend not in libvirt. after user migrate from xm to
xl, he probably use the similar way. Acctually, our customer report this
bug because libvirt libxl driver still missing some apis compare with xl.
raising this message could let user know the circumstances.


regards


bamvor


> If you want to make
> this fail, then a safety catch might be OK but it should be in xl not
> libxl and I think the manipulation should be allowed by default.
>
> Ian.

Re: [PATCH RFC] improve the error message in "xl list --long"

[Ian Jackson]

Bamvor Jian Zhang writes ("Re: [Xen-devel] [PATCH RFC] improve the error message in "xl list --long""):
> considering this case, the user use virt-manager for management the virtual
> machine, he would not aware of xm, xl or virsh is used by virt-manager and
> vm-install. and it is safe to use the xm with virt-manager because the vm
> info is only stored in xend not in libvirt. after user migrate from xm to
> xl, he probably use the similar way. Acctually, our customer report this
> bug because libvirt libxl driver still missing some apis compare with xl.
> raising this message could let user know the circumstances.

Thanks for the explanation.  I'm not sure I entirely follow, so I'm
going to try to explain in my own words what I think you're saying:

virt-manager users are used to using both xm and virsh to manage
guests.  After moving to libxl this is even more relevant because of
missing functionality in the libvirt libxl driver.  However, when a
user uses xl on domains managed by virsh, things go wrong.  You want
this to produce a better error message.

Is this right ?  If so I think some more details would be helpful to
know what goes wrong.  It might be possible to make more of this work
better, or to improve error messages some other way.

It would be possible in principle to make xl produce a warning when
operating on a domain not created with xl, by looking for the missing
userdata.  All modern versions of xl create the userdata.

Ian.

Re: [PATCH RFC] improve the error message in "xl list --long"

[Jim Fehlig]

Ian Jackson wrote:
> Bamvor Jian Zhang writes ("Re: [Xen-devel] [PATCH RFC] improve the error message in "xl list --long""):
>   
>> considering this case, the user use virt-manager for management the virtual
>> machine, he would not aware of xm, xl or virsh is used by virt-manager and
>> vm-install. and it is safe to use the xm with virt-manager because the vm
>> info is only stored in xend not in libvirt. after user migrate from xm to
>> xl, he probably use the similar way. Acctually, our customer report this
>> bug because libvirt libxl driver still missing some apis compare with xl.
>> raising this message could let user know the circumstances.
>>     
>
> Thanks for the explanation.  I'm not sure I entirely follow, so I'm
> going to try to explain in my own words what I think you're saying:
>
> virt-manager users are used to using both xm and virsh to manage
> guests.  After moving to libxl this is even more relevant because of
> missing functionality in the libvirt libxl driver.  However, when a
> user uses xl on domains managed by virsh, things go wrong.  You want
> this to produce a better error message.
>   

I think that is a good summary, as I understand it.

> Is this right ?  If so I think some more details would be helpful to
> know what goes wrong.  It might be possible to make more of this work
> better, or to improve error messages some other way.
>   

A user reported the following when doing a xl long list of a domain
created by libvirt

homevhst01:~ # xl list smt
Name ID Mem VCPUs State Time(s)
smt 1 512 1 -b---- 861.4
homevhst01:~ # xl list --long smt
Domain name must be specified.

Hmm, user did provide a domain name.  This is the error Bamvor is trying
to improve.

> It would be possible in principle to make xl produce a warning when
> operating on a domain not created with xl, by looking for the missing
> userdata.

Yep, sounds reasonable to me.

Regards
Jim

Re: [PATCH RFC] improve the error message in "xl list --long"

[Andrew Cooper]

On 17/10/13 17:21, Bamvor Jian Zhang wrote:
> with this patch, xl will raise a proper error when execute "xl list --long"
> for existed domain but no domain configuration found. it would encounter
> if other toolstack(like libvirt) create the vm, xl could saw the domain
> name but could not get the domain configuration.
>
> there is a commit a76377f1 check the return value of
> libxl_read_file_contents in libxl_userdata_retrieve. but skip the ENOENT.
> it is almost three years before. i do not know the senario about that
> commit. it seems that it is not safe to simplely remove the ENOENT.
>
> Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com>
> ---
>  tools/libxl/libxl_dom.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 356f920..8daf51a 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1655,6 +1655,12 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
>          goto out;
>      }
>  
> +    if (access(filename, F_OK) != 0) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Unknown domain configuration. Is domain id '%u' owned by another libxl toolstack?", domid);

This line is too long,  perhaps putting the start of the string on a new
line, and splitting again at the %u so the grepable parts of the string
stay contiguous.

Finally, "another libxl toolstack" is too specific (XAPI domains being a
prime example which would fall over here).  I would suggest just
"another toolstack".

~Andrew

> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
> +
>      e = libxl_read_file_contents(ctx, filename, data_r ? &data : 0, &datalen);
>      if (e && errno != ENOENT) {
>          rc = ERROR_FAIL;