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