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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 13 Dec 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 15/15] libxl: New event 
> generation API"):
> > +#define GC_INIT(ctx)  libxl__gc *gc = libxl__init_gc(ctx); if (gc == NULL) 
> > return ERROR_NOMEM;
> 
> I think we should avoid macros which return from their containing
> function, unless they are unavoidable.
> 
> Only event-generating libxl entrypoints need to know about the special
> rules for handling the lock and ctx.  The set of event-generating
> entrypoints to libxl is bounded - they are all part of the event
> generation core.
> 
> Also, what you are doing here complicates the usual, simple, case.
> I would prefer to keep the case that can be simple, simple.
> 
> So I think in this case it is better to document the restrictions
> rather than try to abolish them with additional behind-the-scenes
> machinery.

I would agree with you if you were not about to introduce an even more
complicate machinery in the library. A machinery that needs a different
entry point compared to regular libxl functions. Actually I think that
introducing different modes of operations in the same codebase is very
confusing for everyone that works on it. No matter how well you document
them. The kernel is very well documented, but still it is difficult to
understand when you can sleep and when you cannot.
So I am up for anything that would remove this duality.

Also it is very hard to predict whether the event-generating entrypoints
are going to stay bounded.


That said, there is a very easy solution to this problem: we remove the
macro, that I personally dislike anyway, and we open code the check at
the beginning of every function. Now that we are using C99 there isn't
any problem with doing this. It is also much easier to understand what is
going on this way, no hidden behaviors, nothing returning behind your
back, or declaring a new variable a macro.
We could easily make it comply the CODE_STYLE with the addition of a
single line.

@@ -235,7 +241,7 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
 int libxl_domain_rename(libxl_ctx *ctx, uint32_t domid,
                         const char *old_name, const char *new_name)
 {
-    GC_INIT(ctx);
+    libxl__gc *gc = libxl__init_gc(ctx); if (gc == NULL) return ERROR_NOMEM;
     int rc;
     rc = libxl__domain_rename(gc, domid, old_name, new_name, XBT_NULL);
     GC_FREE;



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