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