USB virt 2.6 split driver patch series

USB virt 2.6 split driver patch series

[harry]

I'm about to post a series of 17 patches which implement the xenidc
interdomain communications code and the USB split driver built on the
xenidc code.

These patches have been tested against today's unstable for the i386
build and can be successfully used to create a filesystem on and mount a
USB key.

Currently the code requires swiotlb=force and there are a few issues
which need to be addressed:

o - The USB protocol requires stalling the URB queue during error
conditions.  This isn't done correctly yet which makes the code unsafe
for write access during error recovery.  The 2.4 code had this problem
as well I only discovered it recently.

o - The USB-specific part of the interdomain protocol is using polling
for device discovery which is inefficient.  This is the same strategy as
the 2.4 code.

o - The resource pools are currently only allocating the minimum
resources required to avoid deadlock so performance will be resource
constrained.  This can be fixed either by implementing a larger static
resource allocation or improving the buffer_resource_provider to
implement a dynamic shared resource pool.  Either is fairly easy.

o - I've reformatted all the code to the kernel coding style using
Lindent and not attempted to improve it after that.  Some of the
formatting needs to be improved.

Now that it's basically working, if possible, I'd like to get the code
into the tree and work on it there.

Any feedback on the patches, particularly on what it's going to take to
get the code into the tree, would be most welcome.

Harry.

Re: USB virt 2.6 split driver patch series

[Vincent Hanquez]

On Mon, Nov 21, 2005 at 01:18:24PM +0000, harry wrote:
> o - I've reformatted all the code to the kernel coding style using
> Lindent and not attempted to improve it after that.  Some of the
> formatting needs to be improved.

some ? this is totally _not_ linux kernel coding style.
please have a look at Documentation/CodingStyle and kill anonymous
inline functions (braces in middle of functions).

-- 
Vincent Hanquez

Re: USB virt 2.6 split driver patch series

[harry]

On Mon, 2005-11-21 at 14:49 +0100, Vincent Hanquez wrote:
> On Mon, Nov 21, 2005 at 01:18:24PM +0000, harry wrote:
> > o - I've reformatted all the code to the kernel coding style using
> > Lindent and not attempted to improve it after that.  Some of the
> > formatting needs to be improved.
> 
> some ? this is totally _not_ linux kernel coding style.
> please have a look at Documentation/CodingStyle and kill anonymous
> inline functions (braces in middle of functions).
> 

I have read  Documentation/CodingStyle quite carefully and there is no 
mention of using braces inside functions.  I'm used to using braces to
define minimal scopes for local variables which makes the code easier to
read by minimising the number of variables you need to keep track of
when reading it and by declaring variables closer to where they are used
so it is easier to verify that they have been correctly initialised.

Is this really banned?

Harry.

Re: USB virt 2.6 split driver patch series

[Dave Feustel]

On Monday 21 November 2005 08:49, Vincent Hanquez wrote:
> On Mon, Nov 21, 2005 at 01:18:24PM +0000, harry wrote:
> > o - I've reformatted all the code to the kernel coding style using
> > Lindent and not attempted to improve it after that.  Some of the
> > formatting needs to be improved.
> 
> some ? this is totally _not_ linux kernel coding style.
> please have a look at Documentation/CodingStyle and kill anonymous
> inline functions (braces in middle of functions).

Could you post an example (from the patches, if possible) of 
an "anonymous inline function"?

Thanks,
Dave Feustel
-- 
Switch to Secure OpenBSD with a KDE desktop!!!
NOW with Virtual PC OS support via QEMU and
Beowulf clustering using PETSc and MPICH2!

Re: USB virt 2.6 split driver patch series

[Vincent Hanquez]

On Mon, Nov 21, 2005 at 02:01:01PM +0000, harry wrote:
> I have read  Documentation/CodingStyle quite carefully and there is no 
> mention of using braces inside functions.  I'm used to using braces to
> define minimal scopes for local variables which makes the code easier to
> read by minimising the number of variables you need to keep track of
> when reading it and by declaring variables closer to where they are used
> so it is easier to verify that they have been correctly initialised.
> 
> Is this really banned?

most of the time yes.

the only "good" use of anonymous functions are:

- conditional code, to keep variable definition with the code that
  use it, to prevent "variable not use" warnings.
- modifying some old code ..
   (but most of the time a static inline function is way better,
    unless you are using lots of variable from the function)

plus you'll save lots of blanks at the left of the code and lots of
lines, not using them.

-- 
Vincent Hanquez

Re: USB virt 2.6 split driver patch series

[Dave Feustel]

On Monday 21 November 2005 09:01, harry wrote:
> On Mon, 2005-11-21 at 14:49 +0100, Vincent Hanquez wrote:
> > On Mon, Nov 21, 2005 at 01:18:24PM +0000, harry wrote:
> > > o - I've reformatted all the code to the kernel coding style using
> > > Lindent and not attempted to improve it after that.  Some of the
> > > formatting needs to be improved.
> > 
> > some ? this is totally _not_ linux kernel coding style.
> > please have a look at Documentation/CodingStyle and kill anonymous
> > inline functions (braces in middle of functions).
> > 
> 
> I have read  Documentation/CodingStyle quite carefully and there is no 
> mention of using braces inside functions.  I'm used to using braces to
> define minimal scopes for local variables which makes the code easier to
> read by minimising the number of variables you need to keep track of
> when reading it and by declaring variables closer to where they are used
> so it is easier to verify that they have been correctly initialised.
> 
> Is this really banned?

I thought braces defined code blocks, not internal functions.
Without a label the code can be invoked only inline.

Dave Feustel 
> Harry.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

-- 
Switch to Secure OpenBSD with a KDE desktop!!!
NOW with Virtual PC OS support via QEMU and
Beowulf clustering using PETSc and MPICH2!

Re: USB virt 2.6 split driver patch series

[Vincent Hanquez]

On Mon, Nov 21, 2005 at 09:25:20AM -0500, Dave Feustel wrote:
> Could you post an example (from the patches, if possible) of 
> an "anonymous inline function"?

that what you call code block that are nested into another one
without having any do, while, if, for ...

they are almost like an inlined function in term of code and as they
don't got a label, that make them anonymous.

example:

int fct(void)
{
	int i;

	...
	{
		int j;
		...
	}
	...
}

the patches contains lots of them everywhere.

-- 
Vincent Hanquez

Re: USB virt 2.6 split driver patch series

[NAHieu]

On 11/21/05, harry <harry@hebutterworth.freeserve.co.uk> wrote:
> On Mon, 2005-11-21 at 14:49 +0100, Vincent Hanquez wrote:
> > On Mon, Nov 21, 2005 at 01:18:24PM +0000, harry wrote:
> > > o - I've reformatted all the code to the kernel coding style using
> > > Lindent and not attempted to improve it after that.  Some of the
> > > formatting needs to be improved.
> >
> > some ? this is totally _not_ linux kernel coding style.
> > please have a look at Documentation/CodingStyle and kill anonymous
> > inline functions (braces in middle of functions).
> >
>
> I have read  Documentation/CodingStyle quite carefully and there is no
> mention of using braces inside functions.  I'm used to using braces to
> define minimal scopes for local variables which makes the code easier to
> read by minimising the number of variables you need to keep track of
> when reading it and by declaring variables closer to where they are used
> so it is easier to verify that they have been correctly initialised.
>
> Is this really banned?
>

I guess people complain because this is not a common practice. At
least I never seen any code like that in the kernel.

You could remove them because your functions are pretty short, and it
easy enough to follow the code even if you put all the variables at
the top of functions.


Hieu

Re: USB virt 2.6 split driver patch series

[harry]

On Mon, 2005-11-21 at 23:41 +0900, NAHieu wrote:
> On 11/21/05, harry <harry@hebutterworth.freeserve.co.uk> wrote:
> > On Mon, 2005-11-21 at 14:49 +0100, Vincent Hanquez wrote:
> > > On Mon, Nov 21, 2005 at 01:18:24PM +0000, harry wrote:
> > > > o - I've reformatted all the code to the kernel coding style using
> > > > Lindent and not attempted to improve it after that.  Some of the
> > > > formatting needs to be improved.
> > >
> > > some ? this is totally _not_ linux kernel coding style.
> > > please have a look at Documentation/CodingStyle and kill anonymous
> > > inline functions (braces in middle of functions).
> > >
> >
> > I have read  Documentation/CodingStyle quite carefully and there is no
> > mention of using braces inside functions.  I'm used to using braces to
> > define minimal scopes for local variables which makes the code easier to
> > read by minimising the number of variables you need to keep track of
> > when reading it and by declaring variables closer to where they are used
> > so it is easier to verify that they have been correctly initialised.
> >
> > Is this really banned?
> >
> 
> I guess people complain because this is not a common practice. At
> least I never seen any code like that in the kernel.
> 
> You could remove them because your functions are pretty short, and it
> easy enough to follow the code even if you put all the variables at
> the top of functions.

Yes, I can do this if necessary.  I like to put a trace point at the
start of functions before declaring varibles because some variable
initialisations result in calls to other functions and having the
tracepoint first gets the trace log in the correct order.  The
alternative is to put the declaration first and then initialise after
the trace point.  I'm not used to 8 space tabs either.  The braces for
nested scope don't really work well with such big tabs.

Lindent also messed up by putting the starting open brace for a number
of functions after the parameters rather than at the start of a new
line.  There are also some problems from Lindent with parameter lists
being formatted in a confusing way.  I need to go through and fix these
issues.  I thought it was more important to get the code out for more
exposure and feedback on other issues than hide it away for another week
to fix the formatting perfectly.

> 
> 
> Hieu
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

Re: USB virt 2.6 split driver patch series

[Dave Feustel]

On Monday 21 November 2005 09:36, Vincent Hanquez wrote:
> On Mon, Nov 21, 2005 at 09:25:20AM -0500, Dave Feustel wrote:
> > Could you post an example (from the patches, if possible) of 
> > an "anonymous inline function"?
> 
> that what you call code block that are nested into another one
> without having any do, while, if, for ...
> 
> they are almost like an inlined function in term of code and as they
> don't got a label, that make them anonymous.
> 
> example:
> 
> int fct(void)
> {
> 	int i;
> 
> 	...
> 	{
> 		int j;
> 		...
> 	}
> 	...
> }
> 
> the patches contains lots of them everywhere.

Thanks for posting the example with explanation.
What about this example bothers you and makes
you think this use of brackets should not be permitted
(if in fact this is your position)?
-- 
Switch to Secure OpenBSD with a KDE desktop!!!
NOW with Virtual PC OS support via QEMU and
Beowulf clustering using PETSc and MPICH2!

Re: USB virt 2.6 split driver patch series

[NAHieu]

On 11/22/05, harry <harry@hebutterworth.freeserve.co.uk> wrote:
> On Mon, 2005-11-21 at 23:41 +0900, NAHieu wrote:
> > On 11/21/05, harry <harry@hebutterworth.freeserve.co.uk> wrote:
> > > On Mon, 2005-11-21 at 14:49 +0100, Vincent Hanquez wrote:
> > > > On Mon, Nov 21, 2005 at 01:18:24PM +0000, harry wrote:
> > > > > o - I've reformatted all the code to the kernel coding style using
> > > > > Lindent and not attempted to improve it after that.  Some of the
> > > > > formatting needs to be improved.
> > > >
> > > > some ? this is totally _not_ linux kernel coding style.
> > > > please have a look at Documentation/CodingStyle and kill anonymous
> > > > inline functions (braces in middle of functions).
> > > >
> > >
> > > I have read  Documentation/CodingStyle quite carefully and there is no
> > > mention of using braces inside functions.  I'm used to using braces to
> > > define minimal scopes for local variables which makes the code easier to
> > > read by minimising the number of variables you need to keep track of
> > > when reading it and by declaring variables closer to where they are used
> > > so it is easier to verify that they have been correctly initialised.
> > >
> > > Is this really banned?
> > >
> >
> > I guess people complain because this is not a common practice. At
> > least I never seen any code like that in the kernel.
> >
> > You could remove them because your functions are pretty short, and it
> > easy enough to follow the code even if you put all the variables at
> > the top of functions.
>
> Yes, I can do this if necessary.  I like to put a trace point at the
> start of functions before declaring varibles because some variable
> initialisations result in calls to other functions and having the
> tracepoint first gets the trace log in the correct order.  The
> alternative is to put the declaration first and then initialise after
> the trace point.  I'm not used to 8 space tabs either.  The braces for
> nested scope don't really work well with such big tabs.

why you care about 8 space tabs? Just use tabs to align the code, and
don't pay attention to how many spaces to a tab.

If you think 8 spaces is too long, you could reconfigure your editor.
For example I use vim as editor, and configure it to display 4 spaces
for 1 tab (set shiftwidth=4), and the code looks much better.

>
> Lindent also messed up by putting the starting open brace for a number
> of functions after the parameters rather than at the start of a new
> line.  There are also some problems from Lindent with parameter lists
> being formatted in a confusing way.  I need to go through and fix these
> issues

Beside spliting the code and send them to the list, could you send a
whole in 1 file only? I want to patch it to my tree and give it a try.
Downloading 17 files separately and patch them is tired ;-)

Hieu

Re: USB virt 2.6 split driver patch series

[harry]

On Mon, 2005-11-21 at 17:26 +0000, Oleg Goldshmidt wrote:

> Another style comment that I have is that I think that
> 
> struct xenidc_channel_struct {
> 	void (*submit_message)
> 	 (xenidc_channel * channel, xenidc_channel_message * message);
> 
> is inferior to
> 
> struct xenidc_channel_struct {
> 	void (*submit_message)(xenidc_channel * channel,
> 		               xenidc_channel_message * message);
> 
> in terms of readability.

Thanks.  Yes, I agree. I left it the way that Lindent formatted it but I will do it this way on the manual formatting pass.

Re: USB virt 2.6 split driver patch series

[harry]

[-- Attachment #1: Type: text/plain, Size: 523 bytes --]

On Tue, 2005-11-22 at 00:27 +0900, NAHieu wrote:

> If you think 8 spaces is too long, you could reconfigure your editor.
> For example I use vim as editor, and configure it to display 4 spaces
> for 1 tab (set shiftwidth=4), and the code looks much better.

Thanks, I hadn't thought of that.

> Beside spliting the code and send them to the list, could you send a
> whole in 1 file only? I want to patch it to my tree and give it a try.
> Downloading 17 files separately and patch them is tired ;-)

Here you go...

Harry

[-- Attachment #2: latest-usb-patch.gz --]
[-- Type: application/x-gzip, Size: 94055 bytes --]

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: USB virt 2.6 split driver patch series

[Muli Ben-Yehuda]

On Mon, Nov 21, 2005 at 03:14:10PM +0000, harry wrote:

> Lindent also messed up by putting the starting open brace for a number
> of functions after the parameters rather than at the start of a new
> line.  There are also some problems from Lindent with parameter lists
> being formatted in a confusing way.  I need to go through and fix these
> issues.  I thought it was more important to get the code out for more
> exposure and feedback on other issues than hide it away for another week
> to fix the formatting perfectly.

The thing is, the more readable the coding style, the more meaningful
comments you'll get. I believe readable in this context is the Linux
kernel coding style. Case in point - all of the comments so far have
been about the coding style. Please fix it as soon as possible and
repost so that we could move on to more substantial issues without
getting hung up on the awkward (from a Linux kernel POV!) coding
style.

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: USB virt 2.6 split driver patch series

[Vincent Hanquez]

On Mon, Nov 21, 2005 at 10:24:11AM -0500, Dave Feustel wrote:
> Thanks for posting the example with explanation.
> What about this example bothers you and makes
> you think this use of brackets should not be permitted
> (if in fact this is your position)?

[ oleg's answer is pretty much what I think ]

but yeah, my main reason would be that it deepens the code nesting
unnecessarily; that's really bad that the left half (or more) of the 80
columns is blank.

you end up doing code in the [50:80] range,

like:
					big_name_of_function(do_something,
					                     1 + 1542,
					                     something_else);
instead of:

	big_name_of_function(do_something, 1 + 1542, something_else);

-- 
Vincent Hanquez

Re: USB virt 2.6 split driver patch series

[Dave Feustel]

On Monday 21 November 2005 11:03, Vincent Hanquez wrote:
> On Mon, Nov 21, 2005 at 10:24:11AM -0500, Dave Feustel wrote:
> > Thanks for posting the example with explanation.
> > What about this example bothers you and makes
> > you think this use of brackets should not be permitted
> > (if in fact this is your position)?
> 
> [ oleg's answer is pretty much what I think ]
> 
> but yeah, my main reason would be that it deepens the code nesting
> unnecessarily; that's really bad that the left half (or more) of the 80
> columns is blank.
> 
> you end up doing code in the [50:80] range,
> 
> like:
> 					big_name_of_function(do_something,
> 					                     1 + 1542,
> 					                     something_else);
> instead of:
> 
> 	big_name_of_function(do_something, 1 + 1542, something_else);

I hadn't thought of that, but I have run into exactly the kind of code that you describe. 
It's definitely a PITA to work with.

-- 
Switch to Secure OpenBSD with a KDE desktop!!!
NOW with Virtual PC OS support via QEMU and
Beowulf clustering using PETSc and MPICH2!

Re: USB virt 2.6 split driver patch series

[NAHieu]

On 11/22/05, harry <harry@hebutterworth.freeserve.co.uk> wrote:
> On Tue, 2005-11-22 at 00:27 +0900, NAHieu wrote:
>
> > If you think 8 spaces is too long, you could reconfigure your editor.
> > For example I use vim as editor, and configure it to display 4 spaces
> > for 1 tab (set shiftwidth=4), and the code looks much better.
>
> Thanks, I hadn't thought of that.
>
> > Beside spliting the code and send them to the list, could you send a
> > whole in 1 file only? I want to patch it to my tree and give it a try.
> > Downloading 17 files separately and patch them is tired ;-)
>
> Here you go...
>

Harry, what is the intentional usage of xenidc in drivers/xen/,
besides to support usb driver?

Thanks.
Hieu

Re: USB virt 2.6 split driver patch series

[harry]

On Tue, 2005-11-22 at 02:02 +0900, NAHieu wrote:
> On 11/22/05, harry <harry@hebutterworth.freeserve.co.uk> wrote:
> > On Tue, 2005-11-22 at 00:27 +0900, NAHieu wrote:
> >
> > > If you think 8 spaces is too long, you could reconfigure your editor.
> > > For example I use vim as editor, and configure it to display 4 spaces
> > > for 1 tab (set shiftwidth=4), and the code looks much better.
> >
> > Thanks, I hadn't thought of that.
> >
> > > Beside spliting the code and send them to the list, could you send a
> > > whole in 1 file only? I want to patch it to my tree and give it a try.
> > > Downloading 17 files separately and patch them is tired ;-)
> >
> > Here you go...
> >
> 
> Harry, what is the intentional usage of xenidc in drivers/xen/,
> besides to support usb driver?

It's intended to be generally useful.  So if you wanted to write another
driver you could use the xenidc_endpoint and rbr provider and mapper
pools in the new driver.

The local and remote buffer reference types can be extended as well: the
current code is sufficient for drivers like the USB driver which want to
map kernel virtual address space into the back end.  This would probably
be OK for use by an audio split driver as well and maybe the block
driver.  The network driver or similar drivers which need to swap pages
around would require different local buffer reference and remote buffer
reference types to describe the kind of memory management manipulations
they needed.  It's possible to define and install new types of buffer
references and the generic code will still work with them to do the
resource management etc.

The endpoint code includes the shared page allocation/setup, ring
buffers, interrupt handlers, use of memory barriers, use of grant
tables, some use of xenbus and the set-up/tear-down state machine which
is quite subtle.  Currently this code is replicated in all the other
drivers.  I didn't want another copy in the USB driver so I factored it
all out for common use.

The rbr provider and mapper pool is the bulk data transfer code which
again is replicated in all the other drivers.  Currently the block and
net drivers have different implementations of this but in general there
will be fewer kinds of bulk data transfer required than there will be
drivers so it will be undesirable to have an implementation of bulk data
transfer in every driver.  Again this code is factored out of the USB
driver into a common service.  The common service can be extended for
the different classes of bulk data transfer that will be required for
different classes of device.

> 
> Thanks.
> Hieu
> 

Re: USB virt 2.6 split driver patch series

[Oleg Goldshmidt]

harry <harry@hebutterworth.freeserve.co.uk> writes:

> I have read  Documentation/CodingStyle quite carefully and there is no 
> mention of using braces inside functions.  I'm used to using braces to
> define minimal scopes for local variables which makes the code easier to
> read by minimising the number of variables you need to keep track of
> when reading it and by declaring variables closer to where they are used
> so it is easier to verify that they have been correctly initialised.
> 
> Is this really banned?

Not explicitly, but:

1) It increases the number of almost empty lines, which is explicitly
   frowned upon. An opening brace in an empty line by itself is
   allowed in the beginning of a function body, as per K&R style, and
   it is an exception.

2) It deepens block nesting, which is also explicitly frowned upon.

3) CodingStyle explicitly says that the number of local variables in a
   function should be small (over 5-10 and you are in trouble as far
   as both style and design are concerned). This is a corollary to the
   rule that a function should do one thing and one thing only (but do
   it well). If you follow this rule then introducing blocks to limit
   variable scope is never really justified. If you find yourself in a
   situation where you need such blocking then maybe you should think
   of more modularization (at the function level). Disclaimer: I have
   not studied the patches attentively enough to suggest that you need
   this.

Another style comment that I have is that I think that

struct xenidc_channel_struct {
	void (*submit_message)
	 (xenidc_channel * channel, xenidc_channel_message * message);

is inferior to

struct xenidc_channel_struct {
	void (*submit_message)(xenidc_channel * channel,
		               xenidc_channel_message * message);

in terms of readability.

-- 
Oleg Goldshmidt | pub@NOSPAM.goldshmidt.org | http://www.goldshmidt.org

Re: USB virt 2.6 split driver patch series

[Harry Butterworth]

On Mon, 2005-11-21 at 17:44 +0200, Muli Ben-Yehuda wrote:
> On Mon, Nov 21, 2005 at 03:14:10PM +0000, harry wrote:
> 
> > Lindent also messed up by putting the starting open brace for a number
> > of functions after the parameters rather than at the start of a new
> > line.  There are also some problems from Lindent with parameter lists
> > being formatted in a confusing way.  I need to go through and fix these
> > issues.  I thought it was more important to get the code out for more
> > exposure and feedback on other issues than hide it away for another week
> > to fix the formatting perfectly.
> 
> The thing is, the more readable the coding style, the more meaningful
> comments you'll get. I believe readable in this context is the Linux
> kernel coding style. Case in point - all of the comments so far have
> been about the coding style. Please fix it as soon as possible and
> repost so that we could move on to more substantial issues without
> getting hung up on the awkward (from a Linux kernel POV!) coding
> style.

I'll give it a couple of days to get all the style issue feedback then
fix it in a single pass.  The biggest issue so far seems to be the use
of braces for nested scope which isn't documented in CodingStyle so I
would have had the same feedback even if I had already gone through and
fixed the issues I knew about.

> 
> Cheers,
> Muli
-- 
Harry Butterworth <harry@hebutterworth.freeserve.co.uk>

Re: USB virt 2.6 split driver patch series

[Muli Ben-Yehuda]

On Mon, Nov 21, 2005 at 06:49:27PM +0000, Harry Butterworth wrote:

> I'll give it a couple of days to get all the style issue feedback then
> fix it in a single pass.  The biggest issue so far seems to be the use
> of braces for nested scope which isn't documented in CodingStyle so I
> would have had the same feedback even if I had already gone through and
> fixed the issues I knew about.

CodingStyle is just a rough guide; there's no equivalent to actually
going through some well-written kernel source code and emulating the
style. For example, the blkfront/blkback code looks pretty Linuxish.

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: USB virt 2.6 split driver patch series

[NAHieu]

I had a look at xenidc code, and found some code like this:

--
static void xenidc_endpoint_destroy_1(xenidc_callback * callback)
{
        trace();

        {
                xenidc_endpoint_callback *endpoint_callback =
                    container_of(callback, xenidc_endpoint_callback, callback);

                endpoint_callback->destroyed = 1;

                xenidc_work_wake_up();
        }
}
--

Why name it *destroy_1? it is a common practice to name a local
function with _ or __ as prefix. So for example
xenidc_endpoint_destroy_1() should be named
_xenidc_endpoint_destroy_1() or __xenidc_endpoint_destroy_1()

Hieu

Re: USB virt 2.6 split driver patch series

[NAHieu]

On 11/22/05, NAHieu <nahieu@gmail.com> wrote:
> I had a look at xenidc code, and found some code like this:
>
> --
> static void xenidc_endpoint_destroy_1(xenidc_callback * callback)
> {
>         trace();
>
>         {
>                 xenidc_endpoint_callback *endpoint_callback =
>                     container_of(callback, xenidc_endpoint_callback, callback);
>
>                 endpoint_callback->destroyed = 1;
>
>                 xenidc_work_wake_up();
>         }
> }
> --
>
> Why name it *destroy_1? it is a common practice to name a local
> function with _ or __ as prefix. So for example
> xenidc_endpoint_destroy_1() should be named
> _xenidc_endpoint_destroy_1() or __xenidc_endpoint_destroy_1()
>

Oops, typo. I meant _xenidc_endpoint_destroy_1() should be named
_xenidc_endpoint_destroy() or __xenidc_endpoint_destroy()

Re: USB virt 2.6 split driver patch series

[harry]

On Tue, 2005-11-22 at 11:00 +0900, NAHieu wrote:
> On 11/22/05, NAHieu <nahieu@gmail.com> wrote:
> > I had a look at xenidc code, and found some code like this:
> >
> > --
> > static void xenidc_endpoint_destroy_1(xenidc_callback * callback)
> > {
> >         trace();
> >
> >         {
> >                 xenidc_endpoint_callback *endpoint_callback =
> >                     container_of(callback, xenidc_endpoint_callback, callback);
> >
> >                 endpoint_callback->destroyed = 1;
> >
> >                 xenidc_work_wake_up();
> >         }
> > }
> > --
> >
> > Why name it *destroy_1? it is a common practice to name a local
> > function with _ or __ as prefix. So for example
> > xenidc_endpoint_destroy_1() should be named
> > _xenidc_endpoint_destroy_1() or __xenidc_endpoint_destroy_1()
> >
> 
> Oops, typo. I meant _xenidc_endpoint_destroy_1() should be named
> _xenidc_endpoint_destroy() or __xenidc_endpoint_destroy()
> 

This is for chains of functions which are logically part of the same
operation but are split by asynchronous callbacks. The first function is
called something: xenidc_endpoint_destroy() for example the next
xenidc_endpoint_destroy_1, the next xenidc_endpoint_destroy_2 and so on.
Leading underscores won't work past _1.  Unless you want _ then __ then
___ :-)  Also identifiers with two leading underscores are reserved by
ANSI C for the C compiler implementation so I think it's not a good idea
to use them.

Thanks

Harry.

Re: USB virt 2.6 split driver patch series

[Vincent Hanquez]

On Tue, Nov 22, 2005 at 10:24:29AM +0000, harry wrote:
> Also identifiers with two leading underscores are reserved by
> ANSI C for the C compiler implementation so I think it's not a good idea
> to use them.

This rule is not valid in kernelspace, using 2 leading underscores is
common practice.

-- 
Vincent Hanquez

Re: USB virt 2.6 split driver patch series

[Anthony Liguori]

Vincent Hanquez wrote:

>On Tue, Nov 22, 2005 at 10:24:29AM +0000, harry wrote:
>  
>
>>Also identifiers with two leading underscores are reserved by
>>ANSI C for the C compiler implementation so I think it's not a good idea
>>to use them.
>>    
>>
>
>This rule is not valid in kernelspace, using 2 leading underscores is
>common practice.
>  
>
Harry's right, it's not allowed by the C standard.

FWIW, I don't think it's all that common to use underscores to denote 
functions as private when you can just mark them static.

Regards,

Anthony Liguori

Re: USB virt 2.6 split driver patch series

[Vincent Hanquez]

On Tue, Nov 22, 2005 at 09:37:50AM -0600, Anthony Liguori wrote:
> Harry's right, it's not allowed by the C standard.

still this is common accepted practice is kernel code ...
(no glibc included/linked, gcc specific)

> FWIW, I don't think it's all that common to use underscores to denote 
> functions as private when you can just mark them static.

grep "__[a-z]" | grep "EXPORT" in linux kernel source you'll be surprise.

most kernels-code reason to provide both function (__fctname and fctname) is:
- one is lock free, the other lock what is necessary.
- one test some conditions, the other doesn't

and probably some others reasons ..

Cheers,
-- 
Vincent Hanquez

Re: USB virt 2.6 split driver patch series

[harry]

On Mon, 2005-11-21 at 17:44 +0200, Muli Ben-Yehuda wrote:
> The thing is, the more readable the coding style, the more meaningful
> comments you'll get. I believe readable in this context is the Linux
> kernel coding style. Case in point - all of the comments so far have
> been about the coding style. Please fix it as soon as possible and
> repost so that we could move on to more substantial issues without
> getting hung up on the awkward (from a Linux kernel POV!) coding
> style.

I have now posted REVISION 2 of the first 13 of of the 17 patches which
includes all of the xenidc code.  I have made a best effort attempt at
getting the coding style into line with what I understand to be
acceptable.  As this is my first ever attempt at writing in the Linux
style I expect there are still issues.  I am aware that there are a
couple of places where the code exceeds 80 columns.  I intend to
abbreviate a few function names to fix this.  I'm expecting to have to
do another pass over these patches so further feedback on style is
welcome but hopefully the code is now good enough that it will be
possible to review the content and provide some feedback on that as
well.

I'll follow up with revision 2 of the remaining 4 patches (containing
the actual USB driver) as I reformet them over the next few days.

Harry.