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

[harry <harry@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>]

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