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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-01-24 at 16:23 +0000, Ian Jackson wrote:
> 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:

> > > +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.

The ocaml type of a "builtin" is supplied by the builtin table in the
generator. However:

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

ACK.

> > [...]
> > > +    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.

Usually I agree.

However xl is intended as a libxl testbed as well as a toolstack in its
own right and it is useful to be able to run tools such as valgrind on
it to detect leaks in the library, but this requires not having too many
"false positives" in xl.

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

I did but then I read the below which is entirely correct.

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

That and event overhead generally of having the events registered and
being tracked etc. In the common case we probably have 1 disk and 1
cdrom so we've effectively doubled the amount of stuff we're dealing
with -- whether this becomes significant e.g. on a system with 100+ VMs
I'm not sure.

Ian.


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