WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Harry Butterworth <harry@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
From: Muli Ben-Yehuda <mulix@xxxxxxxxx>
Date: Tue, 22 Nov 2005 12:51:02 +0200
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Tue, 22 Nov 2005 10:51:05 +0000
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1132608080.4739.17.camel@localhost>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1132579119.31295.112.camel@xxxxxxxxxxxxxxxxxxxxx> <20051121201049.GA22728@xxxxxxxxxxxxxxxxxxx> <1132608080.4739.17.camel@localhost>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.11
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