WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

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

<Prev in Thread] Current Thread [Next in Thread>