[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 15/15] libxl: New event generation API
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |