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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 15/15] libxl: New event 
generation API"):
> On Mon, 5 Dec 2011, Ian Jackson wrote:
> > +    LIBXL_TAILQ_INSERT_SORTED(&ctx->death_list, entry, evg, evg_search, ,
> 
> is this a mistake?                                                        ^

No.  It's a parameter to allow the macro's caller to introduce
variable declarations and arbitrary code into the search loop, for the
benefit of their predicate.  We don't need it here.

I've added   /*empty*/  in the empty parameter to avoid people
tripping over it.

> > +void libxl__event_occurred(libxl__gc *gc, libxl_event *event) {
> > +    if (CTX->event_hooks &&
> > +        (CTX->event_hooks->event_occurs_mask & (1UL << event->type))) {
> > +        /* libxl__free_all will call the callback, just before exit
> > +         * from libxl.
> 
> Please extend this comment: "just before leaving libxl to go back to the
> caller".  Also, even though libxl__free_all might be the right place to
> call the callbacks, the name of the function (libxl__free_all) won't
> completely reflect its behaviour anymore. Maybe we need to rename
> libxl__free_all?

Yes, I think you're probably right.  I'll add a patch to rename it to
libxl__gc_cleanup, which I think is the best name I can think of for
the moment.

> > +int libxl_event_check(libxl_ctx *ctx, libxl_event **event_r,
> > +                      unsigned long typemask,
> > +                      libxl_event_predicate *pred, void *pred_user) {
> > +    GC_INIT(ctx);
> > +    CTX_LOCK;
> > +    int rc = event_check_unlocked(gc, event_r, typemask, pred, pred_user);
> > +    CTX_UNLOCK;
> > +    GC_FREE;
> > +    return rc;
> > +}
> 
> I think it is confusing to call event_check_*unlocked* from within
> CTX_LOCK/CTX_UNLOCK.

I'm open to other suggestions for the name of these functions, but I
got the naming pattern from stdio.  From man getc_unlocked(3) on
squeeze:

 UNLOCKED_STDIO(3)          Linux Programmer's Manual         UNLOCKED_STDIO(3)
 ...
        int getc_unlocked(FILE *stream);
        int getchar_unlocked(void);
        int putc_unlocked(int c, FILE *stream);
 ...
 DESCRIPTION
        Each of these functions has the same behavior as its counterpart  with-
        out  the  "_unlocked" suffix, except that they do not use locking (they
        do not set locks themselves, and do not test for the presence of  locks
        set by others) and hence are thread-unsafe.  See flockfile(3).

Now Linux and Xen have a tendency, when they need a name for an
unlocked version of foo, to use the name _foo.  Obviously this is no
good in a userspace program and anyway I think it is a completely
ridiculous convention.  We need something much clearer.

foo_internal is a possibility but that merely suggests that the
GC_INIT has been done, and doesn't make it 100% clear that the caller
must already have done CTX_LOCK.

Thanks,
Ian.

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