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

[Muli Ben-Yehuda <mulix@xxxxxxxxx>]

On Mon, Nov 21, 2005 at 09:21:20PM +0000, Harry Butterworth wrote:
> 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...

You're welcome.

>  while (!list_empty(&serialiser->list) && !serialiser->running) {

Even better.

> > > +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.

Errr... I'm of the opinion that any linux kernel code had to be GPL'd,
but IANAL nor do I play one on TV.

> > > +#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?

My mistake, I meant lose the spaces inside the parens.

> > > +#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.

ok, in that case, it would've been clearer to use C99 structure
initializers, e.g. 

#define XENIDC_CALLBACK_SERIALISER_INIT(name) {
  .foo = SPIN_LOCK_UNLOCKED,
  ...
}

etc.

> > More to come.

FWIW, I plan to wait for an updated version that fixes the superficial
stuff and then get down to actually reviewing the xenidc code.

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