On Tue, 2005-11-22 at 12:51 +0200, Muli Ben-Yehuda wrote:
> 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.
I think that Linux kernel code has to be GPL compatible but if it's not
a derived work of Linux then it doesn't have to be GPL.
I didn't derive the xenidc code from Linux and it's intended to be
useful for use in other OSs too so there's a possibility it could be
licensed differently. I'd have to get approval for my parts and the
other copyright holders would have to agree.
IANAL either.
>
> > > > +#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.
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.
>
> 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,
> ...
> }
OK
>
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home