Re: [Xen-devel] [PATCH 3/9] libxl: New event generation API

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 3/9] libxl: New event generation 
API"):
> On Fri, 2012-01-13 at 19:25 +0000, Ian Jackson wrote:
> > +    free_disable_deaths(gc, &CTX->death_list);
> > +    free_disable_deaths(gc, &CTX->death_reported);
> > +
> > +    libxl_evgen_disk_eject *eject;
> > +    while ((eject = LIBXL_LIST_FIRST(&CTX->disk_eject_evgens)))
> > +        libxl__evdisable_disk_eject(gc, eject);
> 
> Why a helper for deaths but not ejects?

Because the deaths function needs to be called twice, once for each of
two lists.


> > +typedef struct libxl_event_hooks {
> > +    uint64_t event_occurs_mask;
> 
> libxl_event_{wait,check} and LIBXL_EVENTMASK_ALL have an unsigned long
> mask. Are they not the same set of bits?

This is a mistake.  I'll change it to 64 everywhere.


> > + * The user value is returned in the generated events and may be
> > + * used by the caller for whatever it likes.  The type ev_user is
> > + * guaranteed to be an unsigned integer type which is at least
> > + * as big as uint64_t and is also guaranteed to be big enough to
> > + * contain any intptr_t value.
> 
> Does anything actually guarantee that sizeof(uint64_t) >
> sizeof(intptr_t)? I'm sure it's true in practice and I'm happy to rely
> on it. Just interested.

No, nothing does.  If we port this code to a platform where pointers
are more than 64 bits, we will need to change the type of this field
(to make it be an intptr_t or some other type).

> > +libxl_ev_user = UInt(64)
> 
> The other option here would be Builtin(...) and an entry in the builtin
> table in the wrapper generator. 

As I say, that prevents the ocaml idl generator from knowing that the
type is 64 bits and so prevents it from using the right ocaml type.

> Arguably the idl could be improved by causing Number() to have a width
> field. Currently it has a signedness and width is a property of UInt but
> the latter could be pushed up the hierarchy.
> 
> You'd still end up with 
>       FOO = Number("FOO", width=X)
> which isn't really much better.

Indeed.

> Or the ocaml generate could handle Number as the biggest int.

That's a bit wasteful.

> Hrm. None of that seems all that much better than what you have. Chalk
> it up to potential future work.

Right.


> > +libxl_ev_link = Builtin("ev_link", passby=PASS_BY_REFERENCE, private=True)
> > +
> > +libxl_event = Struct("event",[
> > +    ("link",     libxl_ev_link,0),
> 
> This "0" == "const=False" which is the default. I don't think it is
> necessary.

This is a leftover from when there was an in-idl comment parameter.  I
will remove it.

> [...]
> > +    ret = libxl_evenable_domain_death(ctx, domid, 0, &deathw);
> > +    if (ret) goto out;
> > 
> [...]
> > +    if (!diskws) {
> > +        diskws = xmalloc(sizeof(*diskws) * d_config.num_disks);
> 
> I didn't see this getting freed on the exit path.

This is deliberate.  Why bother freeing things when the process is
about to exit.

> > +        for (i = 0; i < d_config.num_disks; i++)
> > +            diskws[i] = NULL;
> > +    }
> > +    for (i = 0; i < d_config.num_disks; i++) {
> > +        ret = libxl_evenable_disk_eject(ctx, domid, d_config.disks[i].vdev,
> > +                                        0, &diskws[i]);
> > +        if (ret) goto out;
> > +    }
> 
> This is all (I think) safe for num_disks == 0 but why waste the effort?

I'm not sure I follow.  Do you think I should put an if() round it,
testing whether d_config.num_disks is nonzero ?

In which case by "effort" do you mean computer effort ?  Surely you
can't mean that because this performance detail of this code is
entirely irrelevant.  But you can't mean human effort in the code
because adding the test would be additional code to write, read,
understand and maintain ...

> Incidentally we have libxl_device_disk.removable which might be an
> opportunity to optimise? Assuming it is meaningful in that way. I think
> in reality only emulated CD-ROM devices ever generate this event but
> perhaps exposing that in the API overcomplicates things.

Optimise to save on pointless xenstore watches you mean ?

Ian.

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