Re: [PATCH][1/17] USB virt 2.6 split driver---xenidc platform

[Harry Butterworth <harry@hebutterworth.freeserve.co.uk>]

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@hebutterworth.freeserve.co.uk>