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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Vincent Hanquez
2005-Nov-21 13:49 UTC
Re: [Xen-devel] USB virt 2.6 split driver patch series
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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! _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Vincent Hanquez
2005-Nov-21 14:27 UTC
Re: [Xen-devel] USB virt 2.6 split driver patch series
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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! _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Vincent Hanquez
2005-Nov-21 14:36 UTC
Re: [Xen-devel] USB virt 2.6 split driver patch series
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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! _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 > issuesBeside 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Muli Ben-Yehuda
2005-Nov-21 15:44 UTC
Re: [Xen-devel] USB virt 2.6 split driver patch series
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/ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Vincent Hanquez
2005-Nov-21 16:03 UTC
Re: [Xen-devel] USB virt 2.6 split driver patch series
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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! _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Oleg Goldshmidt
2005-Nov-21 17:26 UTC
Re: [Xen-devel] USB virt 2.6 split driver patch series
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Harry Butterworth
2005-Nov-21 18:49 UTC
Re: [Xen-devel] USB virt 2.6 split driver patch series
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> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Muli Ben-Yehuda
2005-Nov-21 18:58 UTC
Re: [Xen-devel] USB virt 2.6 split driver patch series
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/ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
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() _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Vincent Hanquez
2005-Nov-22 11:07 UTC
Re: [Xen-devel] USB virt 2.6 split driver patch series
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Anthony Liguori
2005-Nov-22 15:37 UTC
Re: [Xen-devel] USB virt 2.6 split driver patch series
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Vincent Hanquez
2005-Nov-22 20:27 UTC
Re: [Xen-devel] USB virt 2.6 split driver patch series
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel