linux kernel coding style and checkpatch.pl script

linux kernel coding style and checkpatch.pl script

[Tomek The Messenger]

[-- Attachment #1.1: Type: text/plain, Size: 908 bytes --]

Hi
There is checkpatch.pl script where You can check if You wrote code in your
kernel module according to linux kernel style.
However can I ignore warning message?
WARNING: quoted string split across lines
#974: FILE: fpgax67-core.c:974:
+               dev_err(&pdev->dev, "registration not done, driver is
already "
+                                                       "registered\n");

If I don't split line I will have another warning that 80 characters is
exceeded.

For sure I can ignore warnings about:
WARNING: struct  should normally be const
#998: FILE: fpgax67-core.c :998:
+int fpgax67_unregister(struct platform_device *pdev)

For sure all errors must be fixed like:
const char* tmp -> change to -> const char *tmp;
if(  => if (   #insert space

Generally I don't know how much warnings should I correct. If it is
mandatory or only good practise and I can omit some if it doesn't make
sense.

[-- Attachment #1.2: Type: text/html, Size: 1244 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: linux kernel coding style and checkpatch.pl script

[Greg KH]

On Wed, Mar 25, 2020 at 10:36:08AM +0100, Tomek The Messenger wrote:
> Hi
> There is checkpatch.pl script where You can check if You wrote code in your
> kernel module according to linux kernel style.
> However can I ignore warning message?
> WARNING: quoted string split across lines
> #974: FILE: fpgax67-core.c:974:
> +               dev_err(&pdev->dev, "registration not done, driver is
> already "
> +                                                       "registered\n");
> 
> If I don't split line I will have another warning that 80 characters is
> exceeded.

No you should not.

> For sure I can ignore warnings about:
> WARNING: struct  should normally be const
> #998: FILE: fpgax67-core.c :998:
> +int fpgax67_unregister(struct platform_device *pdev)

No, please do not.

> For sure all errors must be fixed like:
> const char* tmp -> change to -> const char *tmp;
> if(  => if (   #insert space

Yes.

> Generally I don't know how much warnings should I correct. If it is
> mandatory or only good practise and I can omit some if it doesn't make
> sense.

If you want your code merged properly, and reviewed, just fix them all,
should not take more than a few hours.

good luck!

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: linux kernel coding style and checkpatch.pl script

[Valdis Klētnieks]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1: Type: text/plain; charset=utf-8, Size: 3457 bytes --]

On Wed, 25 Mar 2020 10:36:08 +0100, Tomek The Messenger said:

> There is checkpatch.pl script

To borrow from Pirates of the Carribean, "They're not exactly rules, they're
more like... suggestions..."

Checkpatch flags *possible* code style problems, but it's not perfect. There's
often good reason to ignore them.   For example:

> However can I ignore warning message?
> WARNING: quoted string split across lines
> #974: FILE: fpgax67-core.c:974:
> +               dev_err(&pdev->dev, "registration not done, driver is already "
> +                                                       "registered\n");
>
> If I don't split line I will have another warning that 80 characters is
> exceeded.

Blech. Ick. <vomiting sounds>

Don't split literal strings, it means that grepping the source tree for
"already registered" fails. Making grep for a string work is more important
than shutting up checkpatch.

> For sure I can ignore warnings about:
> WARNING: struct  should normally be const
> #998: FILE: fpgax67-core.c :998:

This one you actually need to look at what the routine says.  The object is
*usually* a const - but might not be.  Figure out what it actually does.

Always keep in mind that it's a perl script, and just doing regex matching. It
has no clue what the code actually does. Hopefully you have more understanding
of the code than the perl script does...

> For sure all errors must be fixed like:
> const char* tmp -> change to -> const char *tmp;
> if(  => if (   #insert space

If this is new code you're writing, you should fix it before you submit the
patch adding the code. (Unless of course, checkpatch is wrong about a
variable needing to be a const, or similar)

If you're doing major changes to existing code anyhow, a cleanup patch first is
often appropriate.

If you're just fixing checkpatch warnings for the sake of fixing checkpatch
warnings, keep in mind that many maintainers won't accept patches that just
clean up checkpatch, for several reasons:

First, if it's code that's been static for a while (years, sometimes), there's
always a danger that a patch breaks something.  No reason to touch stable code.

If it's code that somebody else is working on, your patch can cause merge
errors with the other person's (which is why only the person doing the other
work should do cleanup patches - they won't conflict with their own work).

Also, it messes up the git history -  consider a patch that changes
an 'if( foo && !bar) {'  to 'if( foo && !baz){'  to fix a bug where the wrong
variable was being tested.  You then submit a patch to fix the space.
Now a 'git blame' on the file shows your patch rather than the one that
fixes the bug.

However, I have it on good authority that Greg KH will cheerfully accept
checkpatch fixes for anything under 'drivers/staging', because that code
is usually in such bad shape that fixups are needed. :)

Personally, I do kernel builds with sparse and extra gcc warnings, and submit
patches only after applying some thought and concluding things like "Yes,
sparse and/or gcc were correct, the variable *should* be static so it can't be
accessed from another module due to a namespace collision".

In other words, the patch should *never* be "Fix a checkpatch/sparse/gcc
complaint". It should always be "Make an objective improvement to the code
that happened to be pointed out by static analysis tools".


[-- Attachment #1.2: Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: linux kernel coding style and checkpatch.pl script

[Anuz Pratap Singh Tomar]

[-- Attachment #1.1: Type: text/plain, Size: 1344 bytes --]

On Wed, Mar 25, 2020 at 9:38 AM Tomek The Messenger <
tomekthemessenger@gmail.com> wrote:

> Hi
> There is checkpatch.pl script where You can check if You wrote code in
> your kernel module according to linux kernel style.
> However can I ignore warning message?
> WARNING: quoted string split across lines
> #974: FILE: fpgax67-core.c:974:
> +               dev_err(&pdev->dev, "registration not done, driver is
> already "
> +                                                       "registered\n");
>
> If I don't split line I will have another warning that 80 characters is
> exceeded.
>
> you can put the whole string on next line and/or use "\" for splitting
long string.

For sure I can ignore warnings about:
> WARNING: struct  should normally be const
> #998: FILE: fpgax67-core.c :998:
> +int fpgax67_unregister(struct platform_device *pdev)
>
> For sure all errors must be fixed like:
> const char* tmp -> change to -> const char *tmp;
> if(  => if (   #insert space
>
> Generally I don't know how much warnings should I correct. If it is
> mandatory or only good practise and I can omit some if it doesn't make
> sense.
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies@kernelnewbies.org
> https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
>


-- 
Thank you
Warm Regards
Anuz

[-- Attachment #1.2: Type: text/html, Size: 2429 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: linux kernel coding style and checkpatch.pl script

[Konstantin Andreev]

Valdis Klētnieks, 26 Mar 2020 07:13 MSK:
> 
> To borrow from Pirates of the Carribean, "They're not exactly rules, they're more like... suggestions..."
> 
> Don't split literal strings, it means that grepping the source tree for "already registered" fails. Making grep for a string work is more important than shutting up checkpatch.

Sic! Grepping is important. Given that, why are kernel functions coded in a

| static int __init loglevel(char *str)
| {

way, but not old decent

| static int __init
| loglevel(char *str)
| {

unix way?

--
Regards, Konstantin

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: linux kernel coding style and checkpatch.pl script

[Greg KH]

On Thu, Mar 26, 2020 at 02:36:23PM +0300, Konstantin Andreev wrote:
> Valdis Klētnieks, 26 Mar 2020 07:13 MSK:
> > 
> > To borrow from Pirates of the Carribean, "They're not exactly rules, they're more like... suggestions..."
> > 
> > Don't split literal strings, it means that grepping the source tree for "already registered" fails. Making grep for a string work is more important than shutting up checkpatch.
> 
> Sic! Grepping is important. Given that, why are kernel functions coded in a
> 
> | static int __init loglevel(char *str)
> | {
> 
> way, but not old decent
> 
> | static int __init
> | loglevel(char *str)
> | {
> 
> unix way?

Documentation/process/coding-style.rst

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: linux kernel coding style and checkpatch.pl script

[Konstantin Andreev]

Valdis Klētnieks, 26 Mar 2020 07:13 MSK:
> Don't split literal strings, it means that grepping the source tree for "already registered" fails. Making grep for a string work is more important than shutting up checkpatch.

On Thu, Mar 26, 2020 at 02:36:23PM +0300, Konstantin Andreev wrote:
> Sic! Grepping is important. Given that, why are kernel functions coded in a
>
> | static int __init loglevel(char *str)
> | {
>
> way, but not old decent unix way:
>
> | static int __init
> | loglevel(char *str)
> | {

Greg KH, 26 Mar 2020 15:06 MSK:
> Documentation/process/coding-style.rst

This document does not answer my question. It does not even require

| static int __init loglevel(char *str)
| {

style. Here is a relevant part of document: "separate functions with one blank line... EXPORT macro should follow closing function brace ... In function prototypes, include parameter names ... E.g.:"

| int system_is_up(void)
| {
|         return system_state == SYSTEM_RUNNING;
| }
| EXPORT_SYMBOL(system_is_up);

That's all. Have I overlooked something? Could you, please, share your own point of view?

-- 
Regards, Konstantin

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: linux kernel coding style and checkpatch.pl script

[Greg KH]

On Thu, Mar 26, 2020 at 04:01:18PM +0300, Konstantin Andreev wrote:
> Valdis Klētnieks, 26 Mar 2020 07:13 MSK:
> > Don't split literal strings, it means that grepping the source tree for "already registered" fails. Making grep for a string work is more important than shutting up checkpatch.
> 
> On Thu, Mar 26, 2020 at 02:36:23PM +0300, Konstantin Andreev wrote:
> > Sic! Grepping is important. Given that, why are kernel functions coded in a
> > 
> > | static int __init loglevel(char *str)
> > | {
> > 
> > way, but not old decent unix way:
> > 
> > | static int __init
> > | loglevel(char *str)
> > | {
> 
> Greg KH, 26 Mar 2020 15:06 MSK:
> > Documentation/process/coding-style.rst
> 
> This document does not answer my question. It does not even require
> 
> | static int __init loglevel(char *str)
> | {
> 
> style. Here is a relevant part of document: "separate functions with one blank line... EXPORT macro should follow closing function brace ... In function prototypes, include parameter names ... E.g.:"
> 
> | int system_is_up(void)
> | {
> |         return system_state == SYSTEM_RUNNING;
> | }
> | EXPORT_SYMBOL(system_is_up);
> 
> That's all. Have I overlooked something? Could you, please, share your own point of view?

If it's not a specific rule in there, then that means you could do
someething crazy like:
	static int __init
	loglevel(char *str)
	{
if you were so loony to do so.

But, it turns out that when you write your code like that, it's harder
to actually find where the function is defined vs. where it is called
when greping a codebase.

That's why those of us who have been at this for a long time prefer:
	static int __init loglevel(char *str)
	{
instead.

much easier to pick out of a list of "where is this defined" output of
'git grep'.

And yes, I know all about ctags and the like, but sometimes you don't
have access to that, or don't want to fire it up and have it pre-process
things just to look at a specific git tree at the moment.

Also, big shoutout to 'vgrep' https://github.com/vrothberg/vgrep which
makes using 'git grep' a zillion times easier and is what I rely on all
the time.

tldr;
	either way is fine, but putting the return value on the function
	name line usually makes it easier for others to find your code.

Hope this helps,

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies