[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
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |