[PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Wei Liu]

The uninitialized domid may cause libxl__domain_make to fail.

In libxl__domain_make:
assert(!libxl_domid_valid_guest(*domid)).

Signed-off-by: Wei Liu <liuw@liuw.name>

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 47a51c8..fbee1d0 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -570,7 +570,7 @@ static int libxl__create_stubdom(libxl__gc *gc,
     libxl_domain_create_info c_info;
     libxl_domain_build_info b_info;
     libxl__domain_build_state state;
-    uint32_t domid;
+    uint32_t domid = 0;
     char **args;
     struct xs_permissions perm[2];
     xs_transaction_t t;

Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Ian Campbell]

On Thu, 2011-06-09 at 06:03 +0100, Wei Liu wrote:
> The uninitialized domid may cause libxl__domain_make to fail.
> 
> In libxl__domain_make:
> assert(!libxl_domid_valid_guest(*domid)).
> 
> Signed-off-by: Wei Liu <liuw@liuw.name>

That check seems pretty odd to me at first but the commit message of
22842:ccfa0527893e does a good job of explaining why so: 

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

although it's not clear why libxl__domain_make doesn't just set an
invalid value as it's first act and save the callers the effort, the net
result would still be the correct semantics for libxl_domid_valid_guest
when the function exits.

Ian.

> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 47a51c8..fbee1d0 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -570,7 +570,7 @@ static int libxl__create_stubdom(libxl__gc *gc,
>      libxl_domain_create_info c_info;
>      libxl_domain_build_info b_info;
>      libxl__domain_build_state state;
> -    uint32_t domid;
> +    uint32_t domid = 0;
>      char **args;
>      struct xs_permissions perm[2];
>      xs_transaction_t t;
> 
> 

Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Wei Liu]

On Thu, 2011-06-09 at 08:55 +0100, Ian Campbell wrote:
> On Thu, 2011-06-09 at 06:03 +0100, Wei Liu wrote:
> > The uninitialized domid may cause libxl__domain_make to fail.
> > 
> > In libxl__domain_make:
> > assert(!libxl_domid_valid_guest(*domid)).
> > 
> > Signed-off-by: Wei Liu <liuw@liuw.name>
> 
> That check seems pretty odd to me at first but the commit message of
> 22842:ccfa0527893e does a good job of explaining why so: 
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> although it's not clear why libxl__domain_make doesn't just set an
> invalid value as it's first act and save the callers the effort, the net
> result would still be the correct semantics for libxl_domid_valid_guest
> when the function exits.
> 

I think the commit message of 22842:ccfa0527893e says pretty clear that
it is caller's responsibility to initialize domid to a invalid value.

However, libxl__make_domain sets domid=-1 a few lines after the check.
This confuses me.

> Ian.
> 

Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Ian Campbell]

On Thu, 2011-06-09 at 09:31 +0100, Wei Liu wrote:
> On Thu, 2011-06-09 at 08:55 +0100, Ian Campbell wrote:
> > On Thu, 2011-06-09 at 06:03 +0100, Wei Liu wrote:
> > > The uninitialized domid may cause libxl__domain_make to fail.
> > > 
> > > In libxl__domain_make:
> > > assert(!libxl_domid_valid_guest(*domid)).
> > > 
> > > Signed-off-by: Wei Liu <liuw@liuw.name>
> > 
> > That check seems pretty odd to me at first but the commit message of
> > 22842:ccfa0527893e does a good job of explaining why so: 
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > although it's not clear why libxl__domain_make doesn't just set an
> > invalid value as it's first act and save the callers the effort, the net
> > result would still be the correct semantics for libxl_domid_valid_guest
> > when the function exits.
> > 
> 
> I think the commit message of 22842:ccfa0527893e says pretty clear that
> it is caller's responsibility to initialize domid to a invalid value.

Only because that's how 22842 causes libxl__make_domain to be
implemented, we are free to change it.

> However, libxl__make_domain sets domid=-1 a few lines after the check.
> This confuses me.

Yeah, me too, that line could just be hoisted up to the first thing the
function does with no loss of safety AFAICT.

Ian.

Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Wei Liu]

On Thu, 2011-06-09 at 11:23 +0100, Ian Campbell wrote:
> On Thu, 2011-06-09 at 09:31 +0100, Wei Liu wrote:
> > On Thu, 2011-06-09 at 08:55 +0100, Ian Campbell wrote:
> > > On Thu, 2011-06-09 at 06:03 +0100, Wei Liu wrote:
> > > > The uninitialized domid may cause libxl__domain_make to fail.
> > > > 
> > > > In libxl__domain_make:
> > > > assert(!libxl_domid_valid_guest(*domid)).
> > > > 
> > > > Signed-off-by: Wei Liu <liuw@liuw.name>
> > > 
> > > That check seems pretty odd to me at first but the commit message of
> > > 22842:ccfa0527893e does a good job of explaining why so: 
> > > 
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > although it's not clear why libxl__domain_make doesn't just set an
> > > invalid value as it's first act and save the callers the effort, the net
> > > result would still be the correct semantics for libxl_domid_valid_guest
> > > when the function exits.
> > > 
> > 
> > I think the commit message of 22842:ccfa0527893e says pretty clear that
> > it is caller's responsibility to initialize domid to a invalid value.
> 
> Only because that's how 22842 causes libxl__make_domain to be
> implemented, we are free to change it.
> 

I'm not against changing it. But I won't do this until I completely
understand what it does and why it does.

My patch is based on similar use case in
libxc_create.c:do_domain_create, which initializes domid to 0 before
calling libxl__domain_make. That's safer (not likely to misunderstand
its usage and introduce new bug) and solve my problem.

> > However, libxl__make_domain sets domid=-1 a few lines after the check.
> > This confuses me.
> 
> Yeah, me too, that line could just be hoisted up to the first thing the
> function does with no loss of safety AFAICT.
> 
> Ian.
> 

Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Ian Campbell]

On Thu, 2011-06-09 at 12:03 +0100, Wei Liu wrote:
> On Thu, 2011-06-09 at 11:23 +0100, Ian Campbell wrote:
> > On Thu, 2011-06-09 at 09:31 +0100, Wei Liu wrote:
> > > On Thu, 2011-06-09 at 08:55 +0100, Ian Campbell wrote:
> > > > On Thu, 2011-06-09 at 06:03 +0100, Wei Liu wrote:
> > > > > The uninitialized domid may cause libxl__domain_make to fail.
> > > > > 
> > > > > In libxl__domain_make:
> > > > > assert(!libxl_domid_valid_guest(*domid)).
> > > > > 
> > > > > Signed-off-by: Wei Liu <liuw@liuw.name>
> > > > 
> > > > That check seems pretty odd to me at first but the commit message of
> > > > 22842:ccfa0527893e does a good job of explaining why so: 
> > > > 
> > > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > > 
> > > > although it's not clear why libxl__domain_make doesn't just set an
> > > > invalid value as it's first act and save the callers the effort, the net
> > > > result would still be the correct semantics for libxl_domid_valid_guest
> > > > when the function exits.
> > > > 
> > > 
> > > I think the commit message of 22842:ccfa0527893e says pretty clear that
> > > it is caller's responsibility to initialize domid to a invalid value.
> > 
> > Only because that's how 22842 causes libxl__make_domain to be
> > implemented, we are free to change it.
> > 
> 
> I'm not against changing it. But I won't do this until I completely
> understand what it does and why it does.

Sure. I think your patch is fine as it is, I was just wondering about
the wider context...

> 
> My patch is based on similar use case in
> libxc_create.c:do_domain_create, which initializes domid to 0 before
> calling libxl__domain_make. That's safer (not likely to misunderstand
> its usage and introduce new bug) and solve my problem.
> 
> > > However, libxl__make_domain sets domid=-1 a few lines after the check.
> > > This confuses me.
> > 
> > Yeah, me too, that line could just be hoisted up to the first thing the
> > function does with no loss of safety AFAICT.
> > 
> > Ian.
> > 
> 
> 

Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Stefano Stabellini]

On Thu, 9 Jun 2011, Ian Campbell wrote:
> On Thu, 2011-06-09 at 09:31 +0100, Wei Liu wrote:
> > On Thu, 2011-06-09 at 08:55 +0100, Ian Campbell wrote:
> > > On Thu, 2011-06-09 at 06:03 +0100, Wei Liu wrote:
> > > > The uninitialized domid may cause libxl__domain_make to fail.
> > > > 
> > > > In libxl__domain_make:
> > > > assert(!libxl_domid_valid_guest(*domid)).
> > > > 
> > > > Signed-off-by: Wei Liu <liuw@liuw.name>
> > > 
> > > That check seems pretty odd to me at first but the commit message of
> > > 22842:ccfa0527893e does a good job of explaining why so: 
> > > 
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > although it's not clear why libxl__domain_make doesn't just set an
> > > invalid value as it's first act and save the callers the effort, the net
> > > result would still be the correct semantics for libxl_domid_valid_guest
> > > when the function exits.
> > > 
> > 
> > I think the commit message of 22842:ccfa0527893e says pretty clear that
> > it is caller's responsibility to initialize domid to a invalid value.
> 
> Only because that's how 22842 causes libxl__make_domain to be
> implemented, we are free to change it.
> 
> > However, libxl__make_domain sets domid=-1 a few lines after the check.
> > This confuses me.
> 
> Yeah, me too, that line could just be hoisted up to the first thing the
> function does with no loss of safety AFAICT.
 
I agree. I think at one point there might have been the expectation that
the domid passed to libxl__domain_make would then would be used in
xc_domain_create.
However it is not the case anymore, so the "safety check" is only
confusing and I think we should remove it.

---

libxl__domain_make: no need to check for domid validness

The domid parameter passed as an argument to libxl__domain_make is an
OUTPUT parameter only.
So there is no need to check for its validity on entry.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff -r 37c77bacb52a tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c	Mon May 23 17:38:28 2011 +0100
+++ b/tools/libxl/libxl_create.c	Thu Jun 09 14:36:25 2011 +0000
@@ -277,8 +277,7 @@ out:
 
 int libxl__domain_make(libxl__gc *gc, libxl_domain_create_info *info,
                        uint32_t *domid)
- /* on entry, libxl_domid_valid_guest(domid) must be false;
-  * on exit (even error exit), domid may be valid and refer to a domain */
+/* on exit (even error exit), domid may be valid and refer to a domain */
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     int flags, ret, i, rc;
@@ -292,8 +291,6 @@ int libxl__domain_make(libxl__gc *gc, li
     xs_transaction_t t = 0;
     xen_domain_handle_t handle;
 
-    assert(!libxl_domid_valid_guest(*domid));
-
     uuid_string = libxl__uuid2string(gc, info->uuid);
     if (!uuid_string) {
         rc = ERROR_NOMEM;

Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Ian Campbell]

On Thu, 2011-06-09 at 15:41 +0100, Stefano Stabellini wrote:
> On Thu, 9 Jun 2011, Ian Campbell wrote:
> > On Thu, 2011-06-09 at 09:31 +0100, Wei Liu wrote:
> > > On Thu, 2011-06-09 at 08:55 +0100, Ian Campbell wrote:
> > > > On Thu, 2011-06-09 at 06:03 +0100, Wei Liu wrote:
> > > > > The uninitialized domid may cause libxl__domain_make to fail.
> > > > > 
> > > > > In libxl__domain_make:
> > > > > assert(!libxl_domid_valid_guest(*domid)).
> > > > > 
> > > > > Signed-off-by: Wei Liu <liuw@liuw.name>
> > > > 
> > > > That check seems pretty odd to me at first but the commit message of
> > > > 22842:ccfa0527893e does a good job of explaining why so: 
> > > > 
> > > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > > 
> > > > although it's not clear why libxl__domain_make doesn't just set an
> > > > invalid value as it's first act and save the callers the effort, the net
> > > > result would still be the correct semantics for libxl_domid_valid_guest
> > > > when the function exits.
> > > > 
> > > 
> > > I think the commit message of 22842:ccfa0527893e says pretty clear that
> > > it is caller's responsibility to initialize domid to a invalid value.
> > 
> > Only because that's how 22842 causes libxl__make_domain to be
> > implemented, we are free to change it.
> > 
> > > However, libxl__make_domain sets domid=-1 a few lines after the check.
> > > This confuses me.
> > 
> > Yeah, me too, that line could just be hoisted up to the first thing the
> > function does with no loss of safety AFAICT.
>  
> I agree. I think at one point there might have been the expectation that
> the domid passed to libxl__domain_make would then would be used in
> xc_domain_create.
> However it is not the case anymore, so the "safety check" is only
> confusing and I think we should remove it.
> 
> ---
> 
> libxl__domain_make: no need to check for domid validness
> 
> The domid parameter passed as an argument to libxl__domain_make is an
> OUTPUT parameter only.
> So there is no need to check for its validity on entry.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> diff -r 37c77bacb52a tools/libxl/libxl_create.c
> --- a/tools/libxl/libxl_create.c	Mon May 23 17:38:28 2011 +0100
> +++ b/tools/libxl/libxl_create.c	Thu Jun 09 14:36:25 2011 +0000
> @@ -277,8 +277,7 @@ out:
>  
>  int libxl__domain_make(libxl__gc *gc, libxl_domain_create_info *info,
>                         uint32_t *domid)
> - /* on entry, libxl_domid_valid_guest(domid) must be false;
> -  * on exit (even error exit), domid may be valid and refer to a domain */
> +/* on exit (even error exit), domid may be valid and refer to a domain */
   /* otherwise libxl_domid_valid_guest(domid) will necessarily be false. */

(just for clarity).

>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      int flags, ret, i, rc;
> @@ -292,8 +291,6 @@ int libxl__domain_make(libxl__gc *gc, li
>      xs_transaction_t t = 0;
>      xen_domain_handle_t handle;
>  
> -    assert(!libxl_domid_valid_guest(*domid));
> -

There's now likely a bunch of needless "domid = 0" in callers but lets
not worry about that now.

>      uuid_string = libxl__uuid2string(gc, info->uuid);
>      if (!uuid_string) {
>          rc = ERROR_NOMEM;

Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Ian Jackson]

Ian Campbell writes ("[Xen-devel] Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom"):
> Yeah, me too, that line could just be hoisted up to the first thing the
> function does with no loss of safety AFAICT.

The question is really what the error handling pattern is expected to
be in the caller.  Our usual error handling pattern (which I think is
a good one) is something like:

   char *something = NULL;
   uint32_t domid = -1;

   ...
   something = allocate();
   if (!something) goto error_exit;
   ...
   ret = libxl__domain_make(&domid);
   if (ret) goto error_exit;
   ...

   return successfully somehow;

  error_exit:
   free(something);
   if (libxl_domid_valid_guest(domid))
       libxl_domain_destroy(domid);

If we expect callers of libxl__domain_make to do this with the domid
then there is no benefit to doing the initialisation in
libxl__domain_make; indeed doing so would just mask
failure-to-initialise bugs in the caller - bugs which the compiler
can't spot.

Defining the interface to libxl__domain_make to _not_ initialise the
value means that the bug of writing
   uint32_t domid;
rather than
   uint32_t domid = -1;
can be detected by valgrind at least and also standds a chance of
failing the assertion.

Ian.

Re: Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Stefano Stabellini]

On Fri, 17 Jun 2011, Ian Jackson wrote:
> Ian Campbell writes ("[Xen-devel] Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom"):
> > Yeah, me too, that line could just be hoisted up to the first thing the
> > function does with no loss of safety AFAICT.
> 
> The question is really what the error handling pattern is expected to
> be in the caller.  Our usual error handling pattern (which I think is
> a good one) is something like:
> 
>    char *something = NULL;
>    uint32_t domid = -1;
> 
>    ...
>    something = allocate();
>    if (!something) goto error_exit;
>    ...
>    ret = libxl__domain_make(&domid);
>    if (ret) goto error_exit;
>    ...
> 
>    return successfully somehow;
> 
>   error_exit:
>    free(something);
>    if (libxl_domid_valid_guest(domid))
>        libxl_domain_destroy(domid);
> 
> If we expect callers of libxl__domain_make to do this with the domid
> then there is no benefit to doing the initialisation in
> libxl__domain_make; indeed doing so would just mask
> failure-to-initialise bugs in the caller - bugs which the compiler
> can't spot.
> 
> Defining the interface to libxl__domain_make to _not_ initialise the
> value means that the bug of writing
>    uint32_t domid;
> rather than
>    uint32_t domid = -1;
> can be detected by valgrind at least and also standds a chance of
> failing the assertion.

If we decide to make domid an output parameter only, then

uint32_t domid;

isn't a bug anymore.
Have you read http://marc.info/?l=xen-devel&m=130763064528133?

Re: Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[ZhouPeng]

[-- Attachment #1: Type: text/plain, Size: 1776 bytes --]

I think it should be both an input and output parameter,
which allows caller/user to provide a given domid,
if the given domid <= 0, it meas to request the hypervisor
to assign the next free id.

so  "assert(!libxl_domid_valid_guest(*domid));" is necessary
and '*domid = -1;' should be cut out. in libxl__domain_make

If any mistake, pls fix me

My patch for this:
---
libxl: fix domid check err.
It should meet the XEN_DOMCTL_createdomain hypercall

Signed-off-by: Zhou Peng <zhoupeng@nfs.iscas.ac.cn>

diff -r eca057e4475c tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c        Fri Jun 17 08:08:13 2011 +0100
+++ b/tools/libxl/libxl_create.c        Tue Jun 21 11:02:51 2011 +0800
@@ -295,7 +295,8 @@ int libxl__domain_make(libxl__gc *gc, li
     xs_transaction_t t = 0;
     xen_domain_handle_t handle;

-    assert(!libxl_domid_valid_guest(*domid));
+    if (*domid > 0)
+        assert(!libxl_domid_valid_guest(*domid));

     uuid_string = libxl__uuid2string(gc, info->uuid);
     if (!uuid_string) {
@@ -306,7 +307,8 @@ int libxl__domain_make(libxl__gc *gc, li
     flags = info->hvm ? XEN_DOMCTL_CDF_hvm_guest : 0;
     flags |= info->hap ? XEN_DOMCTL_CDF_hap : 0;
     flags |= info->oos ? 0 : XEN_DOMCTL_CDF_oos_off;
-    *domid = -1;
+    if (*domid < 0)
+        *domid = -1;

     /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
     libxl_uuid_copy((libxl_uuid *)handle, &info->uuid);


2011/6/21 Stefano Stabellini <stefano.stabellini@eu.citrix.com>:
> If we decide to make domid an output parameter only, then
>
> uint32_t domid;
>
> isn't a bug anymore.
> Have you read http://marc.info/?l=xen-devel&m=130763064528133?

-- 
Zhou Peng
Operating System Technology Group
Institute of Software, the Chinese Academy of Sciences (ISCAS)

[-- Attachment #2: libxl-fix-domid-check-err --]
[-- Type: application/octet-stream, Size: 1088 bytes --]

libxl: fix domid check err.
It should meet the XEN_DOMCTL_createdomain hypercall

Signed-off-by: Zhou Peng <zhoupeng@nfs.iscas.ac.cn>

diff -r eca057e4475c tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c        Fri Jun 17 08:08:13 2011 +0100
+++ b/tools/libxl/libxl_create.c        Tue Jun 21 11:02:51 2011 +0800
@@ -295,7 +295,8 @@ int libxl__domain_make(libxl__gc *gc, li
     xs_transaction_t t = 0;
     xen_domain_handle_t handle;
 
-    assert(!libxl_domid_valid_guest(*domid));
+    if (*domid > 0)
+        assert(!libxl_domid_valid_guest(*domid));
 
     uuid_string = libxl__uuid2string(gc, info->uuid);
     if (!uuid_string) {
@@ -306,7 +307,8 @@ int libxl__domain_make(libxl__gc *gc, li
     flags = info->hvm ? XEN_DOMCTL_CDF_hvm_guest : 0;
     flags |= info->hap ? XEN_DOMCTL_CDF_hap : 0;
     flags |= info->oos ? 0 : XEN_DOMCTL_CDF_oos_off;
-    *domid = -1;
+    if (*domid < 0)
+        *domid = -1;
 
     /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
     libxl_uuid_copy((libxl_uuid *)handle, &info->uuid);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[ZhouPeng]

[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]

Resend for a mistake.

I think 'domid' should be both an input and output parameter,
which allows caller/user to provide a given domid,
if the given domid <= 0, it meas to request the hypervisor
to assign the next free id.

so  "assert(!libxl_domid_valid_guest(*domid));" is necessary, but
should be "assert(libxl_domid_valid_guest(*domid));"
and '*domid = -1;' should be cut out in libxl__domain_make

If any mistake, pls fix me

My patch for this:
---
libxl: fix domid check err.
It should meet the XEN_DOMCTL_createdomain hypercall

Signed-off-by: Zhou Peng <zhoupeng@nfs.iscas.ac.cn>

diff -r eca057e4475c tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c        Fri Jun 17 08:08:13 2011 +0100
+++ b/tools/libxl/libxl_create.c        Tue Jun 21 11:02:51 2011 +0800
@@ -295,7 +295,8 @@ int libxl__domain_make(libxl__gc *gc, li
    xs_transaction_t t = 0;
    xen_domain_handle_t handle;

-    assert(!libxl_domid_valid_guest(*domid));
+    if (*domid > 0)
+        assert(libxl_domid_valid_guest(*domid));

    uuid_string = libxl__uuid2string(gc, info->uuid);
    if (!uuid_string) {
@@ -306,7 +307,8 @@ int libxl__domain_make(libxl__gc *gc, li
    flags = info->hvm ? XEN_DOMCTL_CDF_hvm_guest : 0;
    flags |= info->hap ? XEN_DOMCTL_CDF_hap : 0;
    flags |= info->oos ? 0 : XEN_DOMCTL_CDF_oos_off;
-    *domid = -1;
+    if (*domid < 0)
+        *domid = -1;

    /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
    libxl_uuid_copy((libxl_uuid *)handle, &info->uuid);

-- 
Zhou Peng
Operating System Technology Group
Institute of Software, the Chinese Academy of Sciences (ISCAS)

[-- Attachment #2: libxl-fix-domid-check-err.diff --]
[-- Type: application/octet-stream, Size: 1087 bytes --]

libxl: fix domid check err.
It should meet the XEN_DOMCTL_createdomain hypercall

Signed-off-by: Zhou Peng <zhoupeng@nfs.iscas.ac.cn>

diff -r eca057e4475c tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c        Fri Jun 17 08:08:13 2011 +0100
+++ b/tools/libxl/libxl_create.c        Tue Jun 21 11:02:51 2011 +0800
@@ -295,7 +295,8 @@ int libxl__domain_make(libxl__gc *gc, li
     xs_transaction_t t = 0;
     xen_domain_handle_t handle;
 
-    assert(!libxl_domid_valid_guest(*domid));
+    if (*domid > 0)
+        assert(libxl_domid_valid_guest(*domid));
 
     uuid_string = libxl__uuid2string(gc, info->uuid);
     if (!uuid_string) {
@@ -306,7 +307,8 @@ int libxl__domain_make(libxl__gc *gc, li
     flags = info->hvm ? XEN_DOMCTL_CDF_hvm_guest : 0;
     flags |= info->hap ? XEN_DOMCTL_CDF_hap : 0;
     flags |= info->oos ? 0 : XEN_DOMCTL_CDF_oos_off;
-    *domid = -1;
+    if (*domid < 0)
+        *domid = -1;
 
     /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
     libxl_uuid_copy((libxl_uuid *)handle, &info->uuid);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom"):
> On Fri, 17 Jun 2011, Ian Jackson wrote:
> >    char *something = NULL;
> >    uint32_t domid = -1;
> > 
> >    ...
> >    something = allocate();
> >    if (!something) goto error_exit;
> >    ...
> >    ret = libxl__domain_make(&domid);
> >    if (ret) goto error_exit;
> >    ...
> > 
> >    return successfully somehow;
> > 
> >   error_exit:
> >    free(something);
> >    if (libxl_domid_valid_guest(domid))
> >        libxl_domain_destroy(domid);
...
> If we decide to make domid an output parameter only, then
> 
> uint32_t domid;
> 
> isn't a bug anymore.

Look at the code above.  Without the initialisation, if allocate()
returns NULL, it calls:
  libxl_domid_valid_guest(UNDEFINED)
If it's an output parameter only then neither valgrind nor the
compiler will ever spot this bug unless allocate() actually fails.

The arrangement with the caller initialising and the check in
libxl__domain_make is there to support this error-handling paradim
(which is a good paradigm because it means you don't have to carefully
match up allocations with frees on every error exit path), and which
tries to give us a chance of spotting missing initialisation bugs.

> Have you read http://marc.info/?l=xen-devel&m=130763064528133?

Yes.  I don't agree.

Ian.

Re: Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Ian Jackson]

ZhouPeng writes ("Re: [Xen-devel] Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom"):
> I think it should be both an input and output parameter,
> which allows caller/user to provide a given domid,
> if the given domid <= 0, it meas to request the hypervisor
> to assign the next free id.

This is not correct.  The hypervisor will always assign the domid and
there is no facility to specify one.

Ian.

Re: Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[ZhouPeng]

Thanks for your reply.

If do_domctl is the necessary path for every dom creation,
it seems allow to specify a domid for
the hypercall do_domctl: XEN_DOMCTL_createdomain

The refered code:
do_domctl(XEN_GUEST_HANDLE(xen_domctl_t) u_domctl)

    case XEN_DOMCTL_createdomain:
    {
        struct domain *d;
        domid_t        dom;
        static domid_t rover = 0;
        unsigned int domcr_flags;

        ret = -EINVAL;
        ...
        dom = op->domain;
        /*** here below, it seem to allow specify domid by caller ***/
        if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) )
        {
            ret = -EINVAL;
            if ( !is_free_domid(dom) )
                break;
        }
        else
        {
            for ( dom = rover + 1; dom != rover; dom++ )
            {
                if ( dom == DOMID_FIRST_RESERVED )
                    dom = 0;
                if ( is_free_domid(dom) )
                    break;
            }

            ret = -ENOMEM;
            if ( dom == rover )
                break;

            rover = dom;
        }

If it's true for this hypercall,
it's that the current tool's implementation hide it.

On Tue, Jun 21, 2011 at 8:28 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> ZhouPeng writes ("Re: [Xen-devel] Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom"):
>> I think it should be both an input and output parameter,
>> which allows caller/user to provide a given domid,
>> if the given domid <= 0, it meas to request the hypervisor
>> to assign the next free id.
>
> This is not correct.  The hypervisor will always assign the domid and
> there is no facility to specify one.
>
> Ian.
>



-- 
Zhou Peng
Operating System Technology Group
Institute of Software, the Chinese Academy of Sciences (ISCAS)

Re: Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Stefano Stabellini]

On Tue, 21 Jun 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom"):
> > On Fri, 17 Jun 2011, Ian Jackson wrote:
> > >    char *something = NULL;
> > >    uint32_t domid = -1;
> > > 
> > >    ...
> > >    something = allocate();
> > >    if (!something) goto error_exit;
> > >    ...
> > >    ret = libxl__domain_make(&domid);
> > >    if (ret) goto error_exit;
> > >    ...
> > > 
> > >    return successfully somehow;
> > > 
> > >   error_exit:
> > >    free(something);
> > >    if (libxl_domid_valid_guest(domid))
> > >        libxl_domain_destroy(domid);
> ...
> > If we decide to make domid an output parameter only, then
> > 
> > uint32_t domid;
> > 
> > isn't a bug anymore.
> 
> Look at the code above.  Without the initialisation, if allocate()
> returns NULL, it calls:
>   libxl_domid_valid_guest(UNDEFINED)
> If it's an output parameter only then neither valgrind nor the
> compiler will ever spot this bug unless allocate() actually fails.
> 
> The arrangement with the caller initialising and the check in
> libxl__domain_make is there to support this error-handling paradim
> (which is a good paradigm because it means you don't have to carefully
> match up allocations with frees on every error exit path), and which
> tries to give us a chance of spotting missing initialisation bugs.

I understand what you mean, but in that case I would rather have the
check right before allocate:

assert(!libxl_domid_valid_guest(*domid));
something = allocate();

in the outer function.
Because libxl__domain_make doesn't have any business in checking for the
validity of an output parameter, it is a layering violation.

Re: Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom"):
> I understand what you mean, but in that case I would rather have the
> check right before allocate:
> 
> assert(!libxl_domid_valid_guest(*domid));
> something = allocate();
>
> in the outer function.

What?  Are you proposing this:

    char *something = NULL;
    uint32_t domid = -1;
 
    ...
    assert(!libxl_domid_valid_guest(*domid));
    assert(!something);
    ...

    something = allocate();
    if (!something) goto error_exit;
    ...
    ret = libxl__domain_make(&domid);
    if (ret) goto error_exit;
    ...
 
    return successfully somehow;
 
   error_exit:
    free(something);
    if (libxl_domid_valid_guest(domid))
        libxl_domain_destroy(domid);

What would be the point of that ?

> Because libxl__domain_make doesn't have any business in checking for the
> validity of an output parameter, it is a layering violation.

I'm proposing that this should be an update parameter.

Ian.

Re: Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Stefano Stabellini]

On Tue, 21 Jun 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom"):
> > I understand what you mean, but in that case I would rather have the
> > check right before allocate:
> > 
> > assert(!libxl_domid_valid_guest(*domid));
> > something = allocate();
> >
> > in the outer function.
> 
> What?  Are you proposing this:
> 
>     char *something = NULL;
>     uint32_t domid = -1;
>  
>     ...
>     assert(!libxl_domid_valid_guest(*domid));
>     assert(!something);
>     ...
> 
>     something = allocate();
>     if (!something) goto error_exit;
>     ...
>     ret = libxl__domain_make(&domid);
>     if (ret) goto error_exit;
>     ...
>  
>     return successfully somehow;
>  
>    error_exit:
>     free(something);
>     if (libxl_domid_valid_guest(domid))
>         libxl_domain_destroy(domid);
> 
> What would be the point of that ?
> 
 
For clarity I'll keep calling this function the "outer" function even
though there are no inner functions in this example.


The problem you are talking about is forgetting to initialize domid in
the outer function then jumping to error_exit because of an error in
allocate(), as a consequence libxl_domid_valid_guest is called passing
an UNDEFINED argument. Am I right?

This problem is caused by the way the outer function is coded and should not
affect the way other functions are coded.
I don't think that editing other independent functions is a good way of
making the outer function more resilient.

The specific problem you mentioned can be solved by simply initializing
domid in the outer function.
If you would like a double check you can introduce an assert in the
outer function; libxl__domain_make is not the right place for it.

Re: Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] Re: [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom"):
> This problem is caused by the way the outer function is coded and should not
> affect the way other functions are coded.

The API of a particular function does not live in isolation.

I think that the inner function's API should make it easy to write the
outer function - and, specifically, make it plausible to spot mistakes
in the outer function.

Ian.