|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 14/15] libxl: New API for providing OS events to libxl
On Fri, 9 Dec 2011, Ian Jackson wrote:
> Firstly, Stefano, please trim your quotes. You don't need to quote
> the whole zillion-line patch. Just quote the bits that are relevant.
> Otherwise people may miss your comments as they page through trying to
> find them.
I personally prefer to keep the full quote so that I can search for
references without having to go back to the other email. However I do
understand that some people don't like this so I'll trim my comment to
your patches in the future.
> > > +int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
> > > + struct pollfd *fds, int *timeout_upd,
> > > + struct timeval now);
> > > + /* The caller should provide beforepoll with some space for libxl's
> > > + * fds, and tell libxl how much space is available by setting *nfds_io.
> ... [ many lines of commentary ]
> > > + * libxl_osevent_beforepoll will only reduce the timeout, naturally.
> > > + */
> >
> > so far we have always added the comment on a function above the
> > declaration of the function; we should keep doing it for consistency
>
> I disagree. That comment style involves either:
>
> 1. Repeating the declaration at the top of the comment (or worse,
> repeating the information in the declaration but in a different
> format, doxygen-style); or
>
> 2. Writing a comment which contains almost entirely forward references
>
> This is not too bad if the comment is a one-liner. But with a big
> comment like this, you end up with something like:
>
> /* The caller should provide beforepoll with some space for libxl's
> * fds, and tell libxl how much space is available by setting *nfds_io.
> * fds points to the start of this space (and fds may be a pointer into
> * a larger array, for example, if the application has some fds of
> * its own that it is interested in).
> *
> * On return *nfds_io will in any case have been updated by libxl
> * according to how many fds libxl wants to poll on.
> *
> * If the space was sufficient, libxl fills in fds[0..<new
> * *nfds_io>] suitably for poll(2), updates *timeout_upd if needed,
> * and returns ok.
> *
> * If space was insufficient, fds[0..<old *nfds_io>] is undefined on
> * return; *nfds_io on return will be greater than the value on
> * entry; *timeout_upd may or may not have been updated; and
> * libxl_osevent_beforepoll returns ERROR_BUFERFULL. In this case
> * the application needs to make more space (enough space for
> * *nfds_io struct pollfd) and then call beforepoll again, before
> * entering poll(2). Typically this will involve calling realloc.
> *
> * The application may call beforepoll with fds==NULL and
> * *nfds_io==0 in order to find out how much space is needed.
> *
> * *timeout_upd is as for poll(2): it's in milliseconds, and
> * negative values mean no timeout (infinity).
> * libxl_osevent_beforepoll will only reduce the timeout, naturally.
> */
> int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
> struct pollfd *fds, int *timeout_upd,
> struct timeval now);
>
> This is very disjointed if you try to read it. You start with
>
> /* The caller should provide beforepoll with some space for libxl's
> * fds, and tell libxl how much space is available by setting *nfds_io.
> * fds points to the start of this space (and fds may be a pointer into
> * a larger array, for example, if the application has some fds of
> * its own that it is interested in).
>
> and the natural reaction is "What caller?? What is this beforepoll and
> *nfds_io of which you speak?? What on earth are we talking about??"
>
> The alternative, repeating the declaration, violates the principle
> that things should be said (and defined) in the code if that's
> possible, rather than having the primary reference be a comment. It
> also violates the principle of trying to avoid putting the same
> information in two places.
I would prefer having a brief explanation of what the parameters are
before the function. At least a few times this what my eyes where
looking for reading this patch.
See for example arch/x86/include/asm/paravirt_types.h in the linux
kernel: the long description of the MACROs is before the MACROs.
> If you want consistency then we should change the coding style to put
> comment about a function or object declaration after the prototype or
> declaration.
I disagree.
> > I see that the implementation of libxl_event_check is actually missing
> > from this patch.
> > Is this patch supposed to compiled, even without the actual libxl_event
> > generation? Or are the two patches have to be committed together?
>
> libxl_event_check is not referred to by the code in this patch. It is
> introduced in 15/15. The comment is indeed not 100% true in this
> patch but it seemed better to provide here a comment explaining how
> things are going to be rather than writing
>
> * This function performs all of the IO and other actions,
> * but this does not currently have any visible effect outside
> * libxl.
>
> in this patch and removing it in the next one.
>
> My series compiles, and indeed is supposed to work, after each patch.
Yes, I think it is better this way.
> > > +typedef struct libxl_osevent_hooks {
> > > + int (*fd_register)(void *uselibxl_event_check.r, int fd, void
> > > **for_app_registration_out,
> > > + short events, void *for_libxl);
> > > + int (*fd_modify)(void *user, int fd, void
> > > **for_app_registration_update,
> > > + short events);
> > > + void (*fd_deregister)(void *user, int fd, void *for_app_registration);
> > > + int (*timeout_register)(void *user, void **for_app_registration_out,
> > > + struct timeval abs, void *for_libxl);
> > > + int (*timeout_modify)(void *user, void **for_app_registration_update,
> > > + struct timeval abs);
> > > + void (*timeout_deregister)(void *user, void *for_app_registration_io);
> > > +} libxl_osevent_hooks;
> > > +
> > > +void libxl_osevent_register_hooks(libxl_ctx *ctx,
> > > + const libxl_osevent_hooks *hooks,
> > > + void *user);
> ...
> > while this description is very verbose, it doesn't contain informations
> > on:
> >
> > - what is void *user;
>
> I would have thought this was obvious. Every situation in C where a
> function pointer is passed needs also to pass a void* (or the
> equivalent) so that some context or dynamic information from the
> original caller can be passed to the inner called function. So user
> is stored by libxl and passed to the hooks.
It might be obvious but it is not written anywhere. Considering the
level of details you went through in the following paragraph, I am
surprised that you left to guessing what this parameter is for. Better
be pedantic than leaving things to imagination.
> > - what is "const libxl_osevent_hooks *hooks", in particular the role of
> > each of these function pointers and the description of their
> > arguments.
>
> The role of these function pointers is this:
>
> > > + /* The application which calls register_fd_hooks promises to
> > > + * maintain a register of fds and timeouts that libxl is interested
> > > + * in, and make calls into libxl (libxl_osevent_occurred_*)
> > > + * when those fd events and timeouts occur. This is more efficient
> > > + * than _beforepoll/_afterpoll if there are many fds (which can
> > > + * happen if the same libxl application is managing many domains).
>
> Ie, this is how libxl tells the application what fds and timeouts
> libxl is interested in.
>
> > If I am a user of the library, how am I supposed to pass as user? and as
> > hooks?
> > I think these few lines should go first, then the description of the
> > contract.
>
> Did you spot this comment, earlier ?
That comment explains what the application promises, not what the
parameters are. However I know what you mean: from that paragraph and
from the name of the parameters it is easy to guess what they are for.
Still I would rather avoid leaving anything to guessing.
> > > + * The second approach uses libxl_osevent_register_hooks and is
> > > + * suitable for programs which are already using a callback-based
> > > + * event library.
>
> libxl_osevent_hooks is for this second approach. If you know what a
> callback-based event library is - particularly if you actually have
> one in front of you - all of this should be obvious. If you don't
> know what a callback-based event library is, then you don't have one
> and you don't want to use this interface.
I disagree. Even if it is the first time one sees a callback-based event
library, one should be able to use it without trouble. If it is too
difficult, maybe it is not designed in the best possible way.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |