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