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

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

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@xxxxxxxxx>
> > > > 
> > > > 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@xxxxxxxxxx>
> > > > 
> > > > 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@xxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel