|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
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".
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?
> +#include "xenidc_callback.h"
> +#include <linux/module.h>
local includes after global includes (and asm after linux)
> + 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)
> +EXPORT_SYMBOL(xenidc_callback_serialiser_function);
EXPORT_SYMBOL_GPL?
> +#if ( defined( CONFIG_XEN_IDC_TRACE ) || defined( XEN_IDC_TRACE ) )
No spaces after and before parens
#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.
> +#define traceonly( S ) S
What is this for? lose the parens.
> +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.
> +typedef struct xenidc_callback_struct xenidc_callback;
no typedef for structs.
> +#define XENIDC_CALLBACK_LINK work.XENIDC_WORK_LINK
Please don't, don't hide structure members and don't name them in all
UPPERCASE.
> +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)
{
{
> +#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?
> + list_add_tail
> + (xenidc_callback_to_link(callback),
&serialiser->list);
This should be
list_add_tail(xenidc_callback_to_link(callback),
&serialiser->list);
> +typedef s32 xenidc_error;
Why? also, why signed, and why just 32 bits even on 64? does it go
over the wire?
> +#define XENIDC_ERROR_SUCCESS ( (xenidc_error)0 )
No parens. Also, can you use errno values rather than inventing your
own?
> +#define XENIDC_WORK_LINK link
Why is the renaming necessary?
> +int xenidc_work_schedule(xenidc_work * work);
The '*' should be aligned to the left.
> +/* 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.
More to come.
Cheers,
Muli
--
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |