Home ›

Proper Linux Kernel Coding Style

Issue 99

From Issue #99
July 2002

Jul 01, 2002  By Greg Kroah-Hartman
 in
Some simple rules, written and unwritten, to ensure that other kernel users understand your work.
No ifdefs in .c Code

With the wide number of different processors, different configuration options and variations of the same base hardware types that Linux runs on, it is easy to start having a lot of #ifdef statements in your code. This is not the proper thing to do. Instead, place the #ifdef in a header file, and provide empty inline functions if the code is not to be included.

As an example, consider the following code in drivers/usb/hid-core.c:

static void hid_process_event (struct hid_device *hid,
                               struct hid_field *field,
                               struct hid_usage *usage,
                               __s32 value)
{
   hid_dump_input(usage, value);
   if (hid->claimed & HID_CLAIMED_INPUT)
         hidinput_hid_event(hid, field, usage, value);
#ifdef CONFIG_USB_HIDDEV
   if (hid->claimed & HID_CLAIMED_HIDDEV)
         hiddev_hid_event(hid, usage->hid, value);
#endif
}

Here the author does not want to call hiddev_hid_event() if a specific configuration option is not enabled. This is because that function will not even be present if the configuration option is not enabled.

To remove this #ifdef, the following change was made to include/linux/hiddev.h:

#ifdef CONFIG_USB_HIDDEV
   extern void hiddev_hid_event (struct hid_device *,
                                 unsigned int usage,
                                 int value);
#else
   static inline void
   hiddev_hid_event (struct hid_device
*hid,
                     unsigned int usage,
                     int value) { }
#endif

Then drivers/usb/hid-core.c was changed to: 

static void hid_process_event
                           (struct hid_device *hid,
                            struct hid_field *field,
                            struct hid_usage *usage,
                            __s32 value)
{
   hid_dump_input(usage, value);
   if (hid->claimed & HID_CLAIMED_INPUT)
         hidinput_hid_event(hid, field, usage, value);
   if (hid->claimed & HID_CLAIMED_HIDDEV)
         hiddev_hid_event(hid, usage->hid, value);
}
If CONFIG_USB_HIDDEV is not enabled, the compiler will replace the call to hiddev_hid_event() with a null function call and then optimize away the if statement entirely. This keeps the code readable and easier to maintain over time.

Conclusion

The Linux kernel consists of a large amount of source code, contributed by hundreds of developers over many years. Because the majority of this code follows some simple and basic style and formatting rules, the ability for people to understand new code quickly has been greatly enhanced. If you want to contribute to this code, please read the CodingStyle guidelines and follow them in your patches and new code. The other unwritten rules can be just as important as the written ones when you are trying to convince people to accept your contributions, and they should be followed just as closely.

This article was based upon a paper and presentation that was created for the 2002 Ottawa Linux Symposium.

Resources

Greg Kroah-Hartman is currently the Linux USB and PCI Hot Plug kernel maintainer. He works for IBM doing various Linux kernel-related things and can be reached at greg@kroah.com.

______________________

Comments

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.

any body know send me

Anonymous's picture
Submitted by Anonymous on Thu, 02/11/2010 - 04:34.

i want a unix programming code for read 10 inputs from file and stored it in other file.

I need one information: What

latha's picture
Submitted by latha (not verified) on Mon, 09/21/2009 - 04:22.

I need one information:
What are the "coding standards" used for developing a linux device driver.
If there are standards where can i get all the standards details?
If those are available in any website .Please let me know.

ifdef badness

Anonymous's picture
Submitted by Anonymous (not verified) on Sun, 03/08/2009 - 17:16.

I don't see ifdef badness as being a binary black/white thing. Just like gotos or almost any other programming construct there are ways to use ifdef that are good or bad.

One or two here and there, used for the purpose of clarity, is potentially a good thing.

If you find you're writing a lot of ifdefs, then it is likely that there is something else you should be doing instead: macros, function calls to a CPU specific layer, or something else.

Same deal with gotos. Use goto here and there to make clearer exit paths is a Good Thing, using them to create complex branching paths is a bad thing.

And btw: there is a good way to make a macro for NULL code - something that is used all over the kernel:

#if ENABLE_BLAAH
#define blaah(x) if(x) printf("blaah %d\n",x)
#else
#define blaah(x) do { } while(0)
#endif

That creates a nice macro that always gets the semicolon logic right.

Is an 'ifdef' in C code really that bad?

David Schwartz's picture
Submitted by David Schwartz (not verified) on Tue, 09/19/2006 - 21:49.

I personally think the 'ifdef' in the C code is vastly superior. For one thing, it makes it obvious to anyone looking at the code that the function is only called if CONFIG_USB_HIDDEV is defined. For another thing, the proposed 'improvement' requires someone to look at the header file to understand the C code, rather than having the C code be self-documenting.

The claimed benefit that the code is more readable is the opposite of the truth. The original code makes it clear that nothing is done if CONFIG_USB_HIDDEV is not defined. The 'improved' code obscures that.

As for it being easier to maintain, I presume this is because the selection of the appropriate hid function is now part of the hid code rather than scattered in all the code that might call the hid code. I do agree that there is some value to this, but weighed against the fact that it obscured under what cases the function does something and under what cases it does not, I think it's just not worth it.

Especially if you try to generalize this. For example, what if 'HID_CLAIMED_HIDDEV' is not defined unless CONFIG_USB_HIDDEV is defined. Would you suggest a 'hid_is_claimed' macro that always returns 'true' or 'false' if 'CONFIG_USB_HIDDEV' is not declared? But then which should it return if it's not even supposed to exist as a test?

And what if instead of 'hid->claimed' it was something like 'is_claimed(hid)'? There might be costs associated with the empty function, and when you put that empty function in the header file, you do not necessarily know about every case that might call it.

This can lead to the ultimate ugliness -- code that has no side-effects around it will call the null function. But code that has side-effects or doesn't want the cost associated with deciding whether or not to call the null function might still need an 'ifdef'. What an unsightly mess that would be.

lots of thinking

Andrew Thompson's picture
Submitted by Andrew Thompson (not verified) on Mon, 01/14/2008 - 13:13.

I see you've done a lot of thinking on this problem, and come up with some logical ideas, but I suspect you've never worked on code with lot's of #ifdefs, especially that needs to be portable to a number of different systems. If there are one or two, it is fine, the problem is when you get a bunch of them they are really hard to read. They don't follow normal indentation rules, so it is hard to see where they end, or what '#else' goes with which '#if'. Scary stuff.

#ifdef section question

Anonymous's picture
Submitted by Anonymous on Thu, 08/22/2002 - 02:00.

Just curious...in the scenario described above for moving the #ifdef out of the c and into the h files....it comments about making a null function.

My question is...Does this when executed still make a function call during execution? Wouldn't this cause a few extra clock ticks that could be done elsewhere? Or does the compiler optimize this out of the equation?

Is the hope to avoid excessive #defines as well?

I suppose defining something else like a

#ifdef CONFIG_USB_HIDDEV

#define HIDDEV_FUNCT extern void hiddev_hid_event (struct hid_device *, unsigned int usage,int value);

#else

#define HIDDEV_FUNCT ;

#endif

and then use HIDDEV_FUNCT in the if previously would be avoided to avoid hiding the actual function.. Is this correct?

Re: #ifdef section question

gregkh's picture
Submitted by gregkh (not verified) on Wed, 11/20/2002 - 03:00.

Yes, the compiler optimizes any "null" inline function calls out of the object code, and will not cause any extra clock ticks.

Also, that #define will not work :)

Hope this helps

Re: #ifdef section question

Olle Bergkvist's picture
Submitted by Olle Bergkvist (not verified) on Thu, 02/14/2008 - 09:45.

What about this one?

#ifdef CONFIG_USB_HIDDEV
   extern void hiddev_hid_event (struct hid_device *,
                                 unsigned int usage,
                                 int value);
#else
#define hiddev_hid_event(hid_device,usage,value) ;
#endif
/* the last semicolon is there to not break the logic if the call is the only statement in a block:
   if(foo)
      hiddev_hid_event (thisdev, somethg, 0);
   do_somethg_else();
would become:
   if(foo)
      do_somethg_else();
*/

Besides, what would you do if you need a return value from such a function?
The method described in the guidelines only works for 'void func(args)' (if i understand correctly).

What would such an empty inline function return?

And what if you have to choose between several different function calls upon a DEFINE. 

   if (hid->claimed & HID_CLAIMED_HIDDEV)
#ifdef CONFIG_USB_HIDDEV
         hiddev_hid_event(hid, usage->hid, value);
#else
         hiddev_hid_other_event_stuff(hid, usage->hid, value);
#endif

This would look like:

   if (hid->claimed & HID_CLAIMED_HIDDEV) {
         hiddev_hid_event(hid, usage->hid, value);
         hiddev_hid_other_event_stuff(hid, usage->hid, value);
   }

which is just weird since the code is NEVER going to behave that way.

And what if you have, say 3 lines of code, ifdefed out. Not only function calls (those can be ifdefed in a header file) but also assignments? Should those 3 lines be moved to a new function, that would be ifdefed away in its header?

Webcast
More Resources
How to Build an Optimal Hadoop Cluster to Store and Maintain Unlimited Amounts of Data Using Microservers

Realizing the promise of Apache® Hadoop® requires the effective deployment of compute, memory, storage and networking to achieve optimal results. With its flexibility and multitude of options, it is easy to over or under provision the server infrastructure, resulting in poor performance and high TCO. Join us for an in depth, technical discussion with industry experts from leading Hadoop and server companies who will provide insights into the key considerations for designing and deploying an optimal Hadoop cluster.

Learn More

Sponsored by AMD

White Paper
More Resources
Red Hat White Paper: Using an Open Source Framework to Catch the Bad Guy

Built-in forensics, incident response, and security with Red Hat Enterprise Linux 6

Every security policy provides guidance and requirements for ensuring adequate protection of information and data, as well as high-level technical and administrative security requirements for a system in a given environment. Traditionally, providing security for a system focuses on the confidentiality of the information on it. However, protecting the data integrity and system and data availability is just as important. For example, when processing United States intelligence information, there are three attributes that require protection: confidentiality, integrity, and availability.

Learn more about catching the bad guy in this free white paper.

Learn More

Sponsored by DLT Solutions