harry
2005-Nov-21 13:18 UTC
[Xen-devel] [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
This patch implements some platform facilities used by the xenidc transport which is in turn used by the USB driver: o - A callback object for asynchronous completion of requests submitted to services. o - A work item object for scheduling tasks which builds on the native linux work queues. o - Some simple trace macros for the xenidc code. o - An error type which is supposed to be platform independent. Signed-off-by: Harry Butterworth <butterwo@uk.ibm.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Muli Ben-Yehuda
2005-Nov-21 20:10 UTC
Re: [Xen-devel] [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
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". On Mon, Nov 21, 2005 at 01:18:39PM +0000, harry wrote:> +/* This program is free software; you can redistribute it and/or modify it */ > +/* under the terms of the GNU General Public License as published by the */ > +/* Free Software Foundation; either version 2 of the License, or (at your */ > +/* option) any later version.*/ Do we really need the GPL in every source file?> +#include "xenidc_callback.h" > +#include <linux/module.h>local includes after global includes (and asm after linux)> + while ((!list_empty(&serialiser->list)) > + && (!serialiser->running) > + ) {This should be while ((!list_empty(&serialiser->list)) && !serialiser->running) { (no paren around a simple expression, fit it all on one line if possible in <80 chars)> +EXPORT_SYMBOL(xenidc_callback_serialiser_function);EXPORT_SYMBOL_GPL?> +#if ( defined( CONFIG_XEN_IDC_TRACE ) || defined( XEN_IDC_TRACE ) )No spaces after and before parens #if (defined(CONFIG_XEN_IDC_TRACE) || defined(XEN_IDC_TRACE))> +#define trace0( format ) \ > +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__ ) > + > +#define trace1( format, a0 ) \ > +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0 ) > + > +#define trace2( format, a0, a1 ) \ > +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0, a1 ) > + > +#define trace3( format, a0, a1, a2 ) \ > +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0, a1, a2 ) > + > +#define trace() trace0( "" )gcc has variable argument support in macros, please use it.> +#define traceonly( S ) SWhat is this for? lose the parens.> +void xenidc_work_wake_up(void) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&xenidc_work_list_lock, flags); > + > + while (!list_empty(&xenidc_work_condition)) { > + list_del_init(xenidc_work_condition.next); > + }No need for braces here.> +typedef struct xenidc_callback_struct xenidc_callback;no typedef for structs.> +#define XENIDC_CALLBACK_LINK work.XENIDC_WORK_LINKPlease don''t, don''t hide structure members and don''t name them in all UPPERCASE.> +static inline void xenidc_callback_init > + (xenidc_callback * callback, xenidc_callback_function *function) { This hsould be static inline void xenid_callback_init(struct xenid_callback* callback, xenidc_callback_function* function) { {> +#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?> + list_add_tail > + (xenidc_callback_to_link(callback),&serialiser->list); This should be list_add_tail(xenidc_callback_to_link(callback), &serialiser->list);> +typedef s32 xenidc_error;Why? also, why signed, and why just 32 bits even on 64? does it go over the wire?> +#define XENIDC_ERROR_SUCCESS ( (xenidc_error)0 )No parens. Also, can you use errno values rather than inventing your own?> +#define XENIDC_WORK_LINK linkWhy is the renaming necessary?> +int xenidc_work_schedule(xenidc_work * work);The ''*'' should be aligned to the left.> +/* from a work item and works even when the condition will only be satisfied */ > +/* by another work item. */ > + > +#define xenidc_work_until( condition ) \ > +do \ > +{ \ > + unsigned long flags; \ > + \ > + spin_lock_irqsave( &xenidc_work_list_lock, flags ); \ > + \ > + for( ; ; ) \ > + { \ > + while \ > + ( \ > + ( !list_empty( &xenidc_work_list ) ) \ > + && \ > + ( !( condition ) ) \ > + ) \ > + { \ > + xenidc_work * work = list_entry \ > + ( \ > + xenidc_work_list.next, \ > + xenidc_work, \ > + link \ > + ); \ > + \ > + list_del_init( &work->link ); \ > + \ > + spin_unlock_irqrestore( &xenidc_work_list_lock, flags ); \ > + \ > + xenidc_work_perform_synchronously( work ); \ > + \ > + spin_lock_irqsave( &xenidc_work_list_lock, flags ); \ > + } \ > + \ > + if( condition ) \ > + { \ > + break; \ > + } \ > + \ > + { \ > + struct list_head link; \ > + \ > + INIT_LIST_HEAD( &link ); \ > + \ > + list_add_tail( &link, &xenidc_work_condition ); \ > + \ > + spin_unlock_irqrestore( &xenidc_work_list_lock, flags ); \ > + \ > + wait_event( xenidc_work_waitqueue, list_empty( &link ) ); \ > + \ > + spin_lock_irqsave( &xenidc_work_list_lock, flags ); \ > + } \ > + } \ > + \ > + spin_unlock_irqrestore( &xenidc_work_list_lock, flags ); \ > +} \ > +while( 0 )Argh! I hate these macros with a passion. We could really use a lambda here, but how about just passing an int (*func)(void* p) instead of the condition and making this into a function? I''m aware of the fact that this is how Linux does it (albeit the macros are not quite this long) but debugging it is awful. More to come. Cheers, Muli -- Muli Ben-Yehuda http://www.mulix.org | http://mulix.livejournal.com/ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stephan Seitz
2005-Nov-21 21:03 UTC
Re: [Xen-devel] [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
Muli Ben-Yehuda schrieb:>>+/* This program is free software; you can redistribute it and/or modify it */ >>+/* under the terms of the GNU General Public License as published by the */ >>+/* Free Software Foundation; either version 2 of the License, or (at your */ >>+/* option) any later version. >> >> >*/ > >Do we really need the GPL in every source file? > >Hi, i''m not in the position to criticise your code here, but from my single point of view, it does make sense to include the GPL or whatever license into every single source file. since every single file _could_ be used in other projects, this makes it very clear to every developer how to handle the ''borrowed'' code. at some projects i was always happy to find some GPL''ed or LGPL''ed code with somewhat trivial but timeconsumpting functions e.g. multibyte_aes256_crypt . if you''re looking for some special function, there''s surely no library for inclusion available :) anyway, a license note in your source files doesn''t harm at all. (well, with some editor you might need to push the page down button twice...) greetings Stephan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Muli Ben-Yehuda
2005-Nov-21 21:07 UTC
Re: [Xen-devel] [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
On Mon, Nov 21, 2005 at 10:03:53PM +0100, Stephan Seitz wrote:> Hi, > > i''m not in the position to criticise your code here, but from my single > point of view, it does make > sense to include the GPL or whatever license into every single > source file.I have nothing against a single line saying that the file is under the GPL, and see the toplevel license or README or whatever for the exact license. But anything else appears to be to be a waste of good bits :-) Cheers, Muli -- Muli Ben-Yehuda http://www.mulix.org | http://mulix.livejournal.com/ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Harry Butterworth
2005-Nov-21 21:21 UTC
Re: [Xen-devel] [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
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...> > On Mon, Nov 21, 2005 at 01:18:39PM +0000, harry wrote: > > > +/* This program is free software; you can redistribute it and/or modify it */ > > +/* under the terms of the GNU General Public License as published by the */ > > +/* Free Software Foundation; either version 2 of the License, or (at your */ > > +/* option) any later version. > */ > > Do we really need the GPL in every source file?I asked my team lead to ask the lawyers what to put at the top of the files. 4 months later I don''t have an official reply ;-)> > > +#include "xenidc_callback.h" > > +#include <linux/module.h> > > local includes after global includes (and asm after linux)OK> > > + while ((!list_empty(&serialiser->list)) > > + && (!serialiser->running) > > + ) { > > This should be > > while ((!list_empty(&serialiser->list)) && !serialiser->running) { > > (no paren around a simple expression, fit it all on one line if > possible in <80 chars)Or: while (!list_empty(&serialiser->list) && !serialiser->running) { ?> > > +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.> > > > +#if ( defined( CONFIG_XEN_IDC_TRACE ) || defined( XEN_IDC_TRACE ) ) > > No spaces after and before parensOK. Lindent must have missed this.> > #if (defined(CONFIG_XEN_IDC_TRACE) || defined(XEN_IDC_TRACE)) > > > +#define trace0( format ) \ > > +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__ ) > > + > > +#define trace1( format, a0 ) \ > > +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0 ) > > + > > +#define trace2( format, a0, a1 ) \ > > +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0, a1 ) > > + > > +#define trace3( format, a0, a1, a2 ) \ > > +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0, a1, a2 ) > > + > > +#define trace() trace0( "" ) > > gcc has variable argument support in macros, please use it.OK> > > +#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?> > > +void xenidc_work_wake_up(void) > > +{ > > + unsigned long flags; > > + > > + spin_lock_irqsave(&xenidc_work_list_lock, flags); > > + > > + while (!list_empty(&xenidc_work_condition)) { > > + list_del_init(xenidc_work_condition.next); > > + } > > No need for braces here.OK.> > > +typedef struct xenidc_callback_struct xenidc_callback; > > no typedef for structs.OK.> > > +#define XENIDC_CALLBACK_LINK work.XENIDC_WORK_LINK > > Please don''t, don''t hide structure members and don''t name them in all > UPPERCASE.I didn''t like this either. The problem is that the linux list code list_for_each_entry needs to access the link field of the structres on the list being iterated over. If the structures on the list have a public link field which is nested in a member structure then you don''t necessarily want to make public which nested structure it is in. So, this is a definition which says that the public link of the callback structure is the one in the work member and this definition can be used in the list_for_each_entry macro. Perhaps there''s a better way of doing this without coupling the code using list_for_each_entry to the implementation of the structure with the public link.> > > +static inline void xenidc_callback_init > > + (xenidc_callback * callback, xenidc_callback_function * > function) { > > This hsould be > > static inline void > xenid_callback_init(struct xenid_callback* callback, > xenidc_callback_function* function) > {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.> > > + list_add_tail > > + (xenidc_callback_to_link(callback), > &serialiser->list); > > This should be > > list_add_tail(xenidc_callback_to_link(callback), > &serialiser->list);OK. Lindent messed up a lot of these.> > > +typedef s32 xenidc_error; > > Why? also, why signed, and why just 32 bits even on 64? does it go > over the wire?Yes, it goes over the wire. Of all the code in the patches, I think I''m least happy about the error codes.> > > +#define XENIDC_ERROR_SUCCESS ( (xenidc_error)0 ) > > No parens. Also, can you use errno values rather than inventing your > own?Maybe. The interdomain error codes are communicated potentially between different operating systems. Making them look obviously different might be an advantage.> > > +#define XENIDC_WORK_LINK link > > Why is the renaming necessary?This is the public link of this structure for use in list_for_each_entry.> > > +int xenidc_work_schedule(xenidc_work * work); > > The ''*'' should be aligned to the left.OK> > > +/* from a work item and works even when the condition will only be satisfied */ > > +/* by another work item. */ > > + > > +#define xenidc_work_until( condition ) \ > > +do \ > > +{ \ > > + unsigned long flags; \ > > + \ > > + spin_lock_irqsave( &xenidc_work_list_lock, flags ); \ > > + \ > > + for( ; ; ) \ > > + { \ > > + while \ > > + ( \ > > + ( !list_empty( &xenidc_work_list ) ) \ > > + && \ > > + ( !( condition ) ) \ > > + ) \ > > + { \ > > + xenidc_work * work = list_entry \ > > + ( \ > > + xenidc_work_list.next, \ > > + xenidc_work, \ > > + link \ > > + ); \ > > + \ > > + list_del_init( &work->link ); \ > > + \ > > + spin_unlock_irqrestore( &xenidc_work_list_lock, flags ); \ > > + \ > > + xenidc_work_perform_synchronously( work ); \ > > + \ > > + spin_lock_irqsave( &xenidc_work_list_lock, flags ); \ > > + } \ > > + \ > > + if( condition ) \ > > + { \ > > + break; \ > > + } \ > > + \ > > + { \ > > + struct list_head link; \ > > + \ > > + INIT_LIST_HEAD( &link ); \ > > + \ > > + list_add_tail( &link, &xenidc_work_condition ); \ > > + \ > > + spin_unlock_irqrestore( &xenidc_work_list_lock, flags ); \ > > + \ > > + wait_event( xenidc_work_waitqueue, list_empty( &link ) ); \ > > + \ > > + spin_lock_irqsave( &xenidc_work_list_lock, flags ); \ > > + } \ > > + } \ > > + \ > > + spin_unlock_irqrestore( &xenidc_work_list_lock, flags ); \ > > +} \ > > +while( 0 ) > > Argh! I hate these macros with a passion. We could really use a lambda > here, but how about just passing an int (*func)(void* p) instead of > the condition and making this into a function? I''m aware of the fact > that this is how Linux does it (albeit the macros are not quite this > long) but debugging it is awful.Yeah, this code sucks. I would have preferred if the underlying Linux code worked differently. I could do the function pointer thing but it makes the client code harder to write.> > More to come. > > Cheers, > Muli-- Harry Butterworth <harry@hebutterworth.freeserve.co.uk> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Anthony Liguori
2005-Nov-22 00:10 UTC
Re: [Xen-devel] [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
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". > >On Mon, Nov 21, 2005 at 01:18:39PM +0000, harry wrote: > > >>+#define trace0( format ) \ >>+printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__ ) >>+ >>+#define trace1( format, a0 ) \ >>+printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0 ) >>+ >>+#define trace2( format, a0, a1 ) \ >>+printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0, a1 ) >>+ >>+#define trace3( format, a0, a1, a2 ) \ >>+printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0, a1, a2 ) >>+ >>+#define trace() trace0( "" ) >> >> > >gcc has variable argument support in macros, please use it. > >Please use the C99 version instead of the GCC version. That would be: #define trace(format, ...) printk(KERN_INFO "xenidc %s: " format "\n", __FUNCTION__, ## __VA_ARGS__) The ''##'' is technically a GCC extension but the old style is deprecated anyway :-) Regards, Anthony Liguori _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Muli Ben-Yehuda
2005-Nov-22 10:51 UTC
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@lists.xensource.com http://lists.xensource.com/xen-devel
harry
2005-Nov-22 11:05 UTC
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@lists.xensource.com http://lists.xensource.com/xen-devel
Mark Williamson
2005-Nov-22 15:05 UTC
Re: [Xen-devel] [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
> 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 agree. Not all the code in Linux is GPL anyhow, there''s some BSD-licensed code in there.> 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.If you haven''t used GPL code that other people have copyright on, then it''s not a derived work in my IANAL opition. If you have, you just need their consent. We did this for the frontend drivers, amongst other stuff, to make it easier for BSD ports to pull in code. Cheers, Mark> 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@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel