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

[Dario Faggioli <raistlin@xxxxxxxx>]

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.

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


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();
    }
    ...
}

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

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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