On Mon, 2005-11-21 at 22:10 +0200, Muli Ben-Yehuda wrote:
> I'm going to give every comment once, not everywhere it
> happens. Please apply as appropriate to all recurring
> occurences. Also, this is my personal opinion of what Linux code
> should like like, please feel free to disagree, provided the
> alternative is just as "Linuxy".
Thanks, this is really good feedback...
>
> On Mon, Nov 21, 2005 at 01:18:39PM +0000, harry wrote:
>
> > +/* This program is free software; you can redistribute it and/or modify it
> > */
> > +/* under the terms of the GNU General Public License as published by the
> > */
> > +/* Free Software Foundation; either version 2 of the License, or (at your
> > */
> > +/* option) any later version.
> */
>
> Do we really need the GPL in every source file?
I asked my team lead to ask the lawyers what to put at the top of the
files. 4 months later I don't have an official reply ;-)
>
> > +#include "xenidc_callback.h"
> > +#include <linux/module.h>
>
> local includes after global includes (and asm after linux)
OK
>
> > + while ((!list_empty(&serialiser->list))
> > + && (!serialiser->running)
> > + ) {
>
> This should be
>
> while ((!list_empty(&serialiser->list)) && !serialiser->running) {
>
> (no paren around a simple expression, fit it all on one line if
> possible in <80 chars)
Or:
while (!list_empty(&serialiser->list) && !serialiser->running) {
?
>
> > +EXPORT_SYMBOL(xenidc_callback_serialiser_function);
>
> EXPORT_SYMBOL_GPL?
I don't care. Actually, If the xenidc stuff is going to be really
useful we might want to get agreement from all the copyright holders to
license it under a different license.
>
>
> > +#if ( defined( CONFIG_XEN_IDC_TRACE ) || defined( XEN_IDC_TRACE ) )
>
> No spaces after and before parens
OK. Lindent must have missed this.
>
> #if (defined(CONFIG_XEN_IDC_TRACE) || defined(XEN_IDC_TRACE))
>
> > +#define trace0( format ) \
> > +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__ )
> > +
> > +#define trace1( format, a0 ) \
> > +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0 )
> > +
> > +#define trace2( format, a0, a1 ) \
> > +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0, a1 )
> > +
> > +#define trace3( format, a0, a1, a2 ) \
> > +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0, a1,
> > a2 )
> > +
> > +#define trace() trace0( "" )
>
> gcc has variable argument support in macros, please use it.
OK
>
> > +#define traceonly( S ) S
>
> What is this for? lose the parens.
For code which should only be compiled in when tracing is turned on.
The parens are required, no?
>
> > +void xenidc_work_wake_up(void)
> > +{
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&xenidc_work_list_lock, flags);
> > +
> > + while (!list_empty(&xenidc_work_condition)) {
> > + list_del_init(xenidc_work_condition.next);
> > + }
>
> No need for braces here.
OK.
>
> > +typedef struct xenidc_callback_struct xenidc_callback;
>
> no typedef for structs.
OK.
>
> > +#define XENIDC_CALLBACK_LINK work.XENIDC_WORK_LINK
>
> Please don't, don't hide structure members and don't name them in all
> UPPERCASE.
I didn't like this either. The problem is that the linux list code
list_for_each_entry needs to access the link field of the structres on
the list being iterated over. If the structures on the list have a
public link field which is nested in a member structure then you don't
necessarily want to make public which nested structure it is in. So,
this is a definition which says that the public link of the callback
structure is the one in the work member and this definition can be used
in the list_for_each_entry macro. Perhaps there's a better way of doing
this without coupling the code using list_for_each_entry to the
implementation of the structure with the public link.
>
> > +static inline void xenidc_callback_init
> > + (xenidc_callback * callback, xenidc_callback_function *
> function) {
>
> This hsould be
>
> static inline void
> xenid_callback_init(struct xenid_callback* callback,
> xenidc_callback_function* function)
> {
OK
> {
>
> > +#define XENIDC_CALLBACK_SERIALISER_INIT( name ) \
> > +{ \
> > + SPIN_LOCK_UNLOCKED, \
> > + LIST_HEAD_INIT( name.list ), \
> > + XENIDC_WORK_INIT \
> > + ( name.work, xenidc_callback_serialiser_function, &name ), \
> > + 0 \
> > +}
>
> Does this really need a macro? I know a bunch of Linux code does it,
> but it's always preferable if you can do it as an inline
> function. Also, what does the SPIN_LOCK_UNLOCKED do there?
This is for initialisation of statics.
i.e.
static xenidc_callback_serialiser fred =
XENIDC_CALLBACK_SERIALISER_INIT( fred );
Can't do this with an inline function.
>
> > + list_add_tail
> > + (xenidc_callback_to_link(callback),
> &serialiser->list);
>
> This should be
>
> list_add_tail(xenidc_callback_to_link(callback),
> &serialiser->list);
OK. Lindent messed up a lot of these.
>
> > +typedef s32 xenidc_error;
>
> Why? also, why signed, and why just 32 bits even on 64? does it go
> over the wire?
Yes, it goes over the wire. Of all the code in the patches, I think I'm
least happy about the error codes.
>
> > +#define XENIDC_ERROR_SUCCESS ( (xenidc_error)0 )
>
> No parens. Also, can you use errno values rather than inventing your
> own?
Maybe. The interdomain error codes are communicated potentially between
different operating systems. Making them look obviously different might
be an advantage.
>
> > +#define XENIDC_WORK_LINK link
>
> Why is the renaming necessary?
This is the public link of this structure for use in
list_for_each_entry.
>
> > +int xenidc_work_schedule(xenidc_work * work);
>
> The '*' should be aligned to the left.
OK
>
> > +/* from a work item and works even when the condition will only be
> > satisfied */
> > +/* by another work item.
> > */
> > +
> > +#define xenidc_work_until( condition ) \
> > +do \
> > +{ \
> > + unsigned long flags; \
> > + \
> > + spin_lock_irqsave( &xenidc_work_list_lock, flags ); \
> > + \
> > + for( ; ; ) \
> > + { \
> > + while \
> > + ( \
> > + ( !list_empty( &xenidc_work_list ) ) \
> > + && \
> > + ( !( condition ) ) \
> > + ) \
> > + { \
> > + xenidc_work * work = list_entry \
> > + ( \
> > + xenidc_work_list.next, \
> > + xenidc_work, \
> > + link \
> > + ); \
> > + \
> > + list_del_init( &work->link ); \
> > + \
> > + spin_unlock_irqrestore( &xenidc_work_list_lock, flags ); \
> > + \
> > + xenidc_work_perform_synchronously( work ); \
> > + \
> > + spin_lock_irqsave( &xenidc_work_list_lock, flags ); \
> > + } \
> > + \
> > + if( condition ) \
> > + { \
> > + break; \
> > + } \
> > + \
> > + { \
> > + struct list_head link; \
> > + \
> > + INIT_LIST_HEAD( &link ); \
> > + \
> > + list_add_tail( &link, &xenidc_work_condition ); \
> > + \
> > + spin_unlock_irqrestore( &xenidc_work_list_lock, flags ); \
> > + \
> > + wait_event( xenidc_work_waitqueue, list_empty( &link ) ); \
> > + \
> > + spin_lock_irqsave( &xenidc_work_list_lock, flags ); \
> > + } \
> > + } \
> > + \
> > + spin_unlock_irqrestore( &xenidc_work_list_lock, flags ); \
> > +} \
> > +while( 0 )
>
> Argh! I hate these macros with a passion. We could really use a lambda
> here, but how about just passing an int (*func)(void* p) instead of
> the condition and making this into a function? I'm aware of the fact
> that this is how Linux does it (albeit the macros are not quite this
> long) but debugging it is awful.
Yeah, this code sucks. I would have preferred if the underlying Linux
code worked differently. I could do the function pointer thing but it
makes the client code harder to write.
>
> More to come.
>
> Cheers,
> Muli
--
Harry Butterworth <harry@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home