Xen devel - Jun 2011 - [PATCH] libxl: initialize domid to 0 in libxl__create_stubdom

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;



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

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;
> 
> 


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

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.
> 



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

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.


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

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.
> 


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

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.
> > 
> 
> 


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

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;

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

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;


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

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.


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

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?

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

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)


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

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)


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

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.

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

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.

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

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)

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

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.

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

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.

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

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.


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

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.

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