Re: [Xen-devel] [PATCH v4] libxl: introduce LIBXL_DOMAIN_TYPE_INVALID

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-05-25 at 11:26 +0100, Dario Faggioli wrote:
> On Fri, 2012-05-25 at 10:44 +0100, Ian Campbell wrote:
> > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > > --- a/tools/libxl/libxl_dm.c
> > > +++ b/tools/libxl/libxl_dm.c
> > > @@ -257,6 +257,10 @@ static char ** libxl__build_device_model
> > >          for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; 
> > > i++)
> > >              flexarray_append(dm_args, b_info->extra_hvm[i]);
> > >          break;
> > > +    case LIBXL_DOMAIN_TYPE_INVALID:
> > > +        LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "invalid domain type");
> > > +        flexarray_free(dm_args);
> > > +        return NULL;
> > 
> > In some cases, like here, the code is entitled to assume that type is
> > either PV or HVM, due to the checks in
> > libxl__domain_build_info_setdefault. I think if we see an invalid here
> > then that is an abort() worthy event.
> > 
> As you wish, although it seems to me that this case above is the only
> possible situation where we are returning NULL from that function.
> Nevertheless, a NULL return value is handled and considered (and
> propagated) as error by the caller. That's why, when Roger suggested
> going this way (i.e., retuning NULL), I liked the idea very much and
> went for it.
> 
> That being said, I'm very very very few confident with these code paths,
> so I'm just thought it might worth pointing the above out, but I'll
> definitely cope with your request if you really thing this is they
> correct thing o do.

It would be a bug, similar to an assertion failure, to get to this point
with b_info->type not equal to either PV or HVM.  This comment about
setdefault in libxl_internal.h explains why:
 *     Idempotently sets any members of "p" which is currently set to
 *     a special value indicating that the defaults should be used
 *     (per libxl_<type>_init) to a specific value.
 *
 *     All libxl API functions are expected to have arranged for this
 *     to be called before using any values within these structures.

so having arranged to call that function at the right time we can assume
that type is a sensible value, and indeed setdefault makes this the
case.

> > There are a bunch of places where we look a b_info->type with if
> > statements instead of switch. Plain ifs are ok, but it might be worth
> > checking the ones with an else clause (not else if) too? I suspect in
> > many cases they are entitled that !HVM == PV due to the setdefault
> > thing.
> > 
> 
> Right, and many of them I can take care of. Perhaps the problem is there
> are some places where the event of libxl__domain_type "failing" (i.e.,
> returning something different from HVM or PV) is somewhat considered
> impossible, or at least neglected, like this one here:
> 
> int libxl_domain_suspend(libxl_ctx *ctx, libxl_domain_suspend_info *info,
>                          uint32_t domid, int fd)
> {
>     GC_INIT(ctx);
>     libxl_domain_type type = libxl__domain_type(gc, domid);
>     int live = info != NULL && info->flags & XL_SUSPEND_LIVE;
>     int debug = info != NULL && info->flags & XL_SUSPEND_DEBUG;
>     int rc = 0;
> 
>     rc = libxl__domain_suspend_common(gc, domid, fd, type, live, debug,
>                                       /* No Remus */ NULL);
> 
>     if (!rc && type == LIBXL_DOMAIN_TYPE_HVM)
>         rc = libxl__domain_save_device_model(gc, domid, fd);
>     GC_FREE;
>     return rc;
> }
> 
> 
> Of course that can be right because of your argument about _sefdefault,
> but I'm not sure how to make sure of that and what to do about them...
> Thoughts?

This one is OK, it doesn't have an else case...

> 
> 
> On a related note, re what we where discussing yesterday on IRC about
> putting a 'default' clause on those switches, there already is some
> heterogeneity here. For example:
> 
> 
> int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid)
> {
>     ...
>     switch (libxl__domain_type(gc, domid)) {
>     case LIBXL_DOMAIN_TYPE_HVM:
>         ...
>         break;
>     case LIBXL_DOMAIN_TYPE_PV:
>         ...
>         break;
>     default:
>         abort();

This seems like it needs a TYPE_INVALID, since libxl__domain_type could
legitimately return it.

>     }
>     ...
> }
> 
> or:
> 
> int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
> {
>     ...
>     else {
>         switch (libxl__domain_type(gc, domid_vm)) {
>         case LIBXL_DOMAIN_TYPE_HVM:
>             ...
>             break;
>         case LIBXL_DOMAIN_TYPE_PV:
>             ...
>             break;
>         case LIBXL_DOMAIN_TYPE_INVALID:
>             ...
>             break;
>         default:
>             abort();
>         }
>     ...
> }
> 
> Should we leave it like this? Is it worth/reasonable to convert all the
> 'default:' into 'case LIBXL_DOMAIN_TYPE_INVALID:' (if yes, what do we do
> with the places that have both? :O) ? Or perhaps vice versa?
> 
> If we can gather consensus on an alternative, I can go ahead and put it
> down all over the places...


> 
> Thanks and Regards,
> Dario
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel