Re: [PATCH] update checkpatch.pl to version 0.03
On Jun 8 2007 18:39, Adrian Bunk wrote: >> > >>> Does the console handle it correctly during boot? >> >> Yes That's most likely because printk() handles neither special chars nor special fun (like ANSI color and movement codes). Hence, we should be safe should there be spurious utf8 output on the console (which is most likely 8-bit before userspace switches it to utf-8). >I'm simply saying that allowing UTF-8 in MODULE_AUTHOR and printk's as >you want to can have unwanted effects. > >And I gave modinfo as an example for a tool that is not yet able to >handle UTF-8 correctly in all cases. > >In my opinion, it's not worth the hassle to allow UTF-8 there. >Feel free to disagree. > >> > So, we had some ISO8859-1 and some UTF-8 in there already. (And as for >> > MODULE_AUTHOR, it should stay there - 'fix' modinfo instead.) (Well, and convert all the MODULE_AUTHORs to utf-8) >> >> Using UTF-8 not 8859-1 for consistency is sensible, especially as 8859-1 >> is obsolete and effectively useless now (although I guess much of the >> '8859-1' in the kernel is 1:1 with 8859-15, which isn't so obsolete but >> is just as useless) > >Agreed, if we allow a non-ASCII charset, it should be UTF-8. Jan -- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Fri, Jun 08, 2007 at 04:42:36PM +0100, Alan Cox wrote: > > >>> Does the console handle it correctly during boot? > > Yes > > > >>> Can all tools that process the syslog cope with it? > > Thats a stupid question. The tools people normally use can just fine. > > > >If you find any source file that contains UTF-8 outside of comments > > >please complain loudly. > > > > I present loudly and proudly (I *don't* complain): > > Point made - Adrian, if the tool complains about UTF-8 in author texts > then its buggy and should not be merged. The fact you have a personal > issue with it is neither here nor there It's not a personal issue. Generally, I like UTF-8. I'm simply saying that allowing UTF-8 in MODULE_AUTHOR and printk's as you want to can have unwanted effects. And I gave modinfo as an example for a tool that is not yet able to handle UTF-8 correctly in all cases. In my opinion, it's not worth the hassle to allow UTF-8 there. Feel free to disagree. > > So, we had some ISO8859-1 and some UTF-8 in there already. (And as for > > MODULE_AUTHOR, it should stay there - 'fix' modinfo instead.) > > Using UTF-8 not 8859-1 for consistency is sensible, especially as 8859-1 > is obsolete and effectively useless now (although I guess much of the > '8859-1' in the kernel is 1:1 with 8859-15, which isn't so obsolete but > is just as useless) Agreed, if we allow a non-ASCII charset, it should be UTF-8. cu Adrian -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
> ./drivers/infiniband/core/multicast.c:UTF-8 Unicode C > program text > ./drivers/infiniband/core/sa.h: UTF-8 Unicode C > program text > ./drivers/infiniband/core/sa_query.c: UTF-8 Unicode C > program text These three seem to be caused by bogus 0xa0 characters that snuck in somehow. I'll push the patch below for 2.6.23: diff --git a/drivers/infiniband/core/multicast.c b/drivers/infiniband/core/multicast.c index 1e13ab4..15b4c4d 100644 --- a/drivers/infiniband/core/multicast.c +++ b/drivers/infiniband/core/multicast.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2006 Intel Corporation. All rights reserved. + * Copyright (c) 2006 Intel Corporation. All rights reserved. * * This software is available to you under a choice of one of two * licenses. You may choose to be licensed under the terms of the GNU diff --git a/drivers/infiniband/core/sa.h b/drivers/infiniband/core/sa.h index 24c93fd..b1d4bbf 100644 --- a/drivers/infiniband/core/sa.h +++ b/drivers/infiniband/core/sa.h @@ -1,6 +1,6 @@ /* * Copyright (c) 2004 Topspin Communications. All rights reserved. - * Copyright (c) 2005 Voltaire, Inc. All rights reserved. + * Copyright (c) 2005 Voltaire, Inc. All rights reserved. * Copyright (c) 2006 Intel Corporation. All rights reserved. * * This software is available to you under a choice of one of two diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c index 6469406..9d3797f 100644 --- a/drivers/infiniband/core/sa_query.c +++ b/drivers/infiniband/core/sa_query.c @@ -1,6 +1,6 @@ /* * Copyright (c) 2004 Topspin Communications. All rights reserved. - * Copyright (c) 2005 Voltaire, Inc. All rights reserved. + * Copyright (c) 2005 Voltaire, Inc. All rights reserved. * Copyright (c) 2006 Intel Corporation. All rights reserved. * * This software is available to you under a choice of one of two - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Fri, 2007-06-08 at 17:16 +0200, Jan Engelhardt wrote: > So, we had some ISO8859-1 and some UTF-8 in there already. (And as for > MODULE_AUTHOR, it should stay there - 'fix' modinfo instead.) Ok. Jon. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
> >>> Does the console handle it correctly during boot? Yes > >>> Can all tools that process the syslog cope with it? Thats a stupid question. The tools people normally use can just fine. > >If you find any source file that contains UTF-8 outside of comments > >please complain loudly. > > I present loudly and proudly (I *don't* complain): Point made - Adrian, if the tool complains about UTF-8 in author texts then its buggy and should not be merged. The fact you have a personal issue with it is neither here nor there > So, we had some ISO8859-1 and some UTF-8 in there already. (And as for > MODULE_AUTHOR, it should stay there - 'fix' modinfo instead.) Using UTF-8 not 8859-1 for consistency is sensible, especially as 8859-1 is obsolete and effectively useless now (although I guess much of the '8859-1' in the kernel is 1:1 with 8859-15, which isn't so obsolete but is just as useless) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Jun 8 2007 16:42, Adrian Bunk wrote:
>On Fri, Jun 08, 2007 at 04:34:01PM +0200, Jesper Juhl wrote:
>> On 08/06/07, Adrian Bunk <[EMAIL PROTECTED]> wrote:
>> [snip]
>>>
>>> It's not only about MODULE_AUTHOR, if you consider it rude to limit
>>> people's names to ASCII, then don't forget that we have printk's like
>>> Linux agpgart interface v0.102 (c) Dave Jones
>>>
>>> What happens if the maintainer changes and it's now
>>> Linux agpgart interface v0.103 (c) Dave Ønes
>>>
>>> Does the console handle it correctly during boot?
>>> Can all tools that process the syslog cope with it?
>>>
>>> Perhaps the answer is in both cases "yes", but it's a completely
>>> untested area.
>>>
>>> We really must have all bugs shaken out and all users using fixed tools
>>> _before_ we can start outputting UTF-8 - limiting people's names to
>>> ASCII in not ideal, but IMHO causing breakages for users is a much
>>> bigger problem.
>>
>> I haven't looked at it in depth yet, but it would seem we already have
>> a few files that need to be looked at with this in mind. Looks like
>> it's not exactely a new problem (although all the following could be
>> in comments of course)...
>>...
>
>They should all be in comments, and all UTF-8 I've ever seen in such
>files was only in comments (mostly author names). So yes, adding UTF-8
>to program code will result in new problems.
>
>If you find any source file that contains UTF-8 outside of comments
>please complain loudly.
I present loudly and proudly (I *don't* complain):
17:11 ichi:/ws/linux-2.6.22-rc4 > find . -iname '*.[ch]' -print0 | xargs -0
grep MODULE_AUTHOR | grep -P '[\x80-\xff]' --color=never
./arch/i386/kernel/cpu/cpufreq/e_powersaver.c:MODULE_AUTHOR("Rafa� Bilski
<[EMAIL PROTECTED]>");
./drivers/char/watchdog/i6300esb.c:MODULE_AUTHOR("Ross Biro and David H�deman");
./drivers/char/watchdog/w83627hf_wdt.c:MODULE_AUTHOR("P�raig Brady <[EMAIL
PROTECTED]>");
./drivers/hwmon/via686a.c:MODULE_AUTHOR("Ky�ti M�kki <[EMAIL PROTECTED]>, "
./drivers/i2c/busses/i2c-via.c:MODULE_AUTHOR("Ky�ti M�kki <[EMAIL PROTECTED]>");
./drivers/input/keyboard/omap-keypad.c:MODULE_AUTHOR("Timo Ter�");
./drivers/isdn/hisax/isdnhdlc.c:MODULE_AUTHOR("Wolfgang M�s <[EMAIL
PROTECTED]>, "
./drivers/mmc/host/omap.c:MODULE_AUTHOR("Juha Yrj��);
./drivers/mtd/devices/phram.c:MODULE_AUTHOR("Jörn Engel <[EMAIL PROTECTED]>");
./drivers/mtd/maps/cfi_flagadm.c:MODULE_AUTHOR("Kári Davíðsson <[EMAIL
PROTECTED]>");
./drivers/mtd/maps/dbox2-flash.c:MODULE_AUTHOR("Kári Davíðsson <[EMAIL
PROTECTED]>, Bastian Blank <[EMAIL PROTECTED]>, Alexander Wild <[EMAIL
PROTECTED]>");
./drivers/net/irda/kingsun-sir.c:MODULE_AUTHOR("Alex Villac�s Lasso <[EMAIL
PROTECTED]>");
./drivers/scsi/aha152x.c:MODULE_AUTHOR("J�gen Fischer");
./drivers/scsi/sni_53c710.c:MODULE_AUTHOR("Thomas Bogend�fer");
./drivers/usb/misc/emi26.c:MODULE_AUTHOR("tapio laxstr�");
./drivers/usb/misc/emi62.c:MODULE_AUTHOR("tapio laxstr�");
So, we had some ISO8859-1 and some UTF-8 in there already. (And as for
MODULE_AUTHOR, it should stay there - 'fix' modinfo instead.)
BTW, there's also still a ton of non-UTF-8 in the kernel; I've already
fixed that weeks ago and sent some patch to trivial@, Adrian -
did you receive it?
Jan
--
Re: [PATCH] update checkpatch.pl to version 0.03
On Fri, Jun 08, 2007 at 04:34:01PM +0200, Jesper Juhl wrote: > On 08/06/07, Adrian Bunk <[EMAIL PROTECTED]> wrote: > [snip] >> >> It's not only about MODULE_AUTHOR, if you consider it rude to limit >> people's names to ASCII, then don't forget that we have printk's like >> Linux agpgart interface v0.102 (c) Dave Jones >> >> What happens if the maintainer changes and it's now >> Linux agpgart interface v0.103 (c) Dave Ønes >> >> Does the console handle it correctly during boot? >> Can all tools that process the syslog cope with it? >> >> Perhaps the answer is in both cases "yes", but it's a completely >> untested area. >> >> We really must have all bugs shaken out and all users using fixed tools >> _before_ we can start outputting UTF-8 - limiting people's names to >> ASCII in not ideal, but IMHO causing breakages for users is a much >> bigger problem. > > I haven't looked at it in depth yet, but it would seem we already have > a few files that need to be looked at with this in mind. Looks like > it's not exactely a new problem (although all the following could be > in comments of course)... >... They should all be in comments, and all UTF-8 I've ever seen in such files was only in comments (mostly author names). So yes, adding UTF-8 to program code will result in new problems. If you find any source file that contains UTF-8 outside of comments please complain loudly. cu Adrian -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On 08/06/07, Adrian Bunk <[EMAIL PROTECTED]> wrote: [snip] It's not only about MODULE_AUTHOR, if you consider it rude to limit people's names to ASCII, then don't forget that we have printk's like Linux agpgart interface v0.102 (c) Dave Jones What happens if the maintainer changes and it's now Linux agpgart interface v0.103 (c) Dave Ønes Does the console handle it correctly during boot? Can all tools that process the syslog cope with it? Perhaps the answer is in both cases "yes", but it's a completely untested area. We really must have all bugs shaken out and all users using fixed tools _before_ we can start outputting UTF-8 - limiting people's names to ASCII in not ideal, but IMHO causing breakages for users is a much bigger problem. I haven't looked at it in depth yet, but it would seem we already have a few files that need to be looked at with this in mind. Looks like it's not exactely a new problem (although all the following could be in comments of course)... $ find ./ -name "*.[ch]" | xargs file | grep -i utf ./arch/arm/mach-pxa/leds-trizeps4.c: UTF-8 Unicode C program text ./arch/arm/mach-pxa/trizeps4.c: UTF-8 Unicode C program text ./arch/powerpc/platforms/cell/spufs/file.c: UTF-8 Unicode C program text ./drivers/acpi/asus_acpi.c: UTF-8 Unicode C program text ./drivers/char/drm/r128_drv.h:UTF-8 Unicode C program text ./drivers/char/drm/radeon_irq.c: UTF-8 Unicode C program text ./drivers/char/drm/drm_drawable.c:UTF-8 Unicode C program text ./drivers/char/drm/drm_pci.c: UTF-8 Unicode C program text ./drivers/char/drm/drm_core.h:UTF-8 Unicode C program text ./drivers/char/hw_random/omap-rng.c: UTF-8 Unicode C program text ./drivers/char/esp.c: UTF-8 Unicode C program text ./drivers/char/watchdog/iTCO_vendor_support.c:UTF-8 Unicode C program text ./drivers/i2c/busses/i2c-iop3xx.c:UTF-8 Unicode C program text ./drivers/infiniband/core/multicast.c:UTF-8 Unicode C program text ./drivers/infiniband/core/sa.h: UTF-8 Unicode C program text ./drivers/infiniband/core/sa_query.c: UTF-8 Unicode C program text ./drivers/mtd/chips/cfi_cmdset_0001.c:UTF-8 Unicode C program text ./drivers/mtd/chips/cfi_probe.c: UTF-8 Unicode C program text ./drivers/mtd/devices/block2mtd.c:UTF-8 Unicode C program text ./drivers/mtd/devices/phram.c:UTF-8 Unicode English text ./drivers/mtd/maps/cfi_flagadm.c: UTF-8 Unicode C program text ./drivers/mtd/maps/dbox2-flash.c: UTF-8 Unicode C program text ./drivers/mtd/maps/mtx-1_flash.c: UTF-8 Unicode C program text ./drivers/mtd/nand/ts7250.c: UTF-8 Unicode C program text ./drivers/mtd/nand/cafe_nand.c: UTF-8 Unicode C program text ./drivers/mtd/nand/cmx270_nand.c: UTF-8 Unicode C program text ./drivers/mtd/nand/cs553x_nand.c: UTF-8 Unicode C program text ./drivers/mtd/nand/edb7312.c: UTF-8 Unicode C program text ./drivers/mtd/nand/h1910.c: UTF-8 Unicode C program text ./drivers/mtd/mtdsuper.c: UTF-8 Unicode C program text ./drivers/mtd/ubi/build.c:UTF-8 Unicode C program text ./drivers/mtd/ubi/cdev.c: UTF-8 Unicode C program text ./drivers/mtd/ubi/debug.c:UTF-8 Unicode C program text ./drivers/mtd/ubi/debug.h:UTF-8 Unicode C program text ./drivers/mtd/ubi/gluebi.c: UTF-8 Unicode C program text ./drivers/mtd/ubi/io.c: UTF-8 Unicode C program text ./drivers/mtd/ubi/kapi.c: UTF-8 Unicode C program text ./drivers/mtd/ubi/misc.c: UTF-8 Unicode C program text ./drivers/mtd/ubi/scan.c: UTF-8 Unicode C program text ./drivers/mtd/ubi/scan.h: UTF-8 Unicode C program text ./drivers/mtd/ubi/ubi.h: UTF-8 Unicode C program text ./drivers/mtd/ubi/upd.c: UTF-8 Unicode C program text ./drivers/mtd/ubi/vmt.c: UTF-8 Unicode C program text ./drivers/mtd/ubi/vtbl.c: UTF-8 Unicode C program text ./drivers/mtd/ubi/wl.c: UTF-8 Unicode C program text ./drivers/mtd
Re: [PATCH] update checkpatch.pl to version 0.03
On Fri, Jun 08, 2007 at 11:52:19AM +0100, Alan Cox wrote: > > The problem is that the second byte is interpreted as a control code. > > > > Is there any trick to get the shell working again in this situation? > > The cursor hangs, and I've not yet found a trick to do anything in this > > xterm again (except for killing it from another xterm). > > For gnome terminal just select 'reset terminal' in the menu or escape-[c; > (from memory) is the VT reset code. If your xterm can be stuck forever > file a security bug against your vendors xterm for a DoS attack problem. Someone else already told me this trick, and the "Full Reset" from the Control + middle mouse button menu works with my xterm. Not a problem if you know about it or if you have time. > > > "Require" is a rather strong word for a print formatting issue specific > > > to obscure setups. > > > > See obove, it's not only "print formatting", it's "kills my shell". > > It printed a symbol, if your shell really got screwed that much by it > then your shell needs work perhaps. My shell is bash... > I'm not btw arguing that we shouldn't > teach the tools to be more polite, just that its hardly a "requirement" For tools like ls or vim that have to deal with every kind of charset confusion for ages such issues have already been shaken out. But tools don't expects the kernel to output non-ASCII strings. It's not only about MODULE_AUTHOR, if you consider it rude to limit people's names to ASCII, then don't forget that we have printk's like Linux agpgart interface v0.102 (c) Dave Jones What happens if the maintainer changes and it's now Linux agpgart interface v0.103 © Dave Ønes Does the console handle it correctly during boot? Can all tools that process the syslog cope with it? Perhaps the answer is in both cases "yes", but it's a completely untested area. We really must have all bugs shaken out and all users using fixed tools _before_ we can start outputting UTF-8 - limiting people's names to ASCII in not ideal, but IMHO causing breakages for users is a much bigger problem. > Alan cu Adrian -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
> The problem is that the second byte is interpreted as a control code. > > Is there any trick to get the shell working again in this situation? > The cursor hangs, and I've not yet found a trick to do anything in this > xterm again (except for killing it from another xterm). For gnome terminal just select 'reset terminal' in the menu or escape-[c; (from memory) is the VT reset code. If your xterm can be stuck forever file a security bug against your vendors xterm for a DoS attack problem. > > "Require" is a rather strong word for a print formatting issue specific > > to obscure setups. > > See obove, it's not only "print formatting", it's "kills my shell". It printed a symbol, if your shell really got screwed that much by it then your shell needs work perhaps. I'm not btw arguing that we shouldn't teach the tools to be more polite, just that its hardly a "requirement" Alan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On 06/08/2007 11:31 AM, Andy Whitcroft wrote:
Ok, the latest version 0.04 is released and I have also put up the
complete script at:
http://www.shadowen.org/~apw/public/checkpatch/checkpatch.pl-0.04
Previous versions are also there.
Thank you. False positive:
do not use assignment in condition
#809: FILE: drivers/cdrom/mitsumi.c:766:
+ while ((req = elv_next_request(q))) {
Doing an assignment in a while (or for) condition like that makes perfect
sense. The check should probably be limited to if conditions -- you can
always split those.
Next:
struct mutex definition without comment
#173: FILE: drivers/cdrom/mitsumi.c:130:
+ struct mutex mutex;
Going overboard. It's the only locking primitive in the driver; the only
comment that could be added is something like "used for mutual exclusion"
which firmly falls into the "i++; /* increase i */" category. A bit further
on up in the driver, a:
/*
* LOCKING: mutex_lock(&mcd->mutex)
*/
is present just before the functions that need to be called with it held
(all, basically). I'd suggest simply dropping this check as its intention is
not something that can be usefully automated I feel. The tree would end up
with tons of useless comments that are there just to shut up the script.
And next a ton of:
labels should not be indented
#249: FILE: drivers/cdrom/mitsumi.c:206:
+ out:
This driver is putting two spaces in front of labels, as explained here:
http://lkml.org/lkml/2007/6/5/61
I do agree that putting them level aligned is a _significantly_ different
style, so perhaps it wants to warn if a label is not within the first indent
level (column 0-7) but if even that's contentious then this check should
perhaps go completely as well. One or two spaces, four for all I care, can
be considered a personal preference I feel.
Rene.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
Rene Herman wrote: > On 06/04/2007 09:08 PM, Andy Whitcroft wrote: > >> I guess line length and white space checks make sense some degree on >> those files. I'll sort that out and I guess we'll have anohter version. > > Could you then also post the thing itself, and not just a patch against > the previous version for us chickens too scared to run -mm? Ok, the latest version 0.04 is released and I have also put up the complete script at: http://www.shadowen.org/~apw/public/checkpatch/checkpatch.pl-0.04 Previous versions are also there. -apw - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Fri, 2007-06-08 02:04:55 +0200, Adrian Bunk <[EMAIL PROTECTED]> wrote: > On Fri, Jun 08, 2007 at 12:41:17AM +0100, Alan Cox wrote: > > (And incidentially since the Linux fs has been defined to be utf-8 for > > naming for many years you'll find the same problem using "ls") > > No, "ls" can handle it perfectly: > > # echo $LANG > C > # ls > ??rsted > # `ls' may be playing tricks by checking whether its output goes to a TTY. Does the terminal also hang for $ ls | cat or $ ls -N MfG, JBG -- Jan-Benedict Glaw [EMAIL PROTECTED] +49-172-7608481 Signature of:http://catb.org/~esr/faqs/smart-questions.html the second : signature.asc Description: Digital signature
Re: [PATCH] update checkpatch.pl to version 0.03
On Fri, 2007-06-08 at 02:04 +0200, Adrian Bunk wrote: > The difference is that "ls" expects and handles such issues while > "lsmod" (and most likely also other userspace working with kernel > output) does not yet handle it resulting in problems if bytes are > wrongly interpreted as control codes. I'm happy to modify module-init-tools for Unicode support, I just didn't think this was a huge issue - but now there's a test case so... :-) Jon. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Fri, Jun 08, 2007 at 12:41:17AM +0100, Alan Cox wrote:
> > I added a MODULE_AUTHOR("J. Ørsted <[EMAIL PROTECTED]>") into the "raw"
> > module:
> >
> > # echo $LANG
> > C
> > # modinfo --version
> > module-init-tools version 3.3-pre11
> > # modinfo raw
> > filename: /lib/modules/2.6.21.2/kernel/drivers/char/raw.ko
> > author: J. Ã
> > ^ the cursor hangs here
>
> "Mummy if I deliberately shoot myself in the head it hurts"
>
> Distro's don't ship in C locale and haven't for years. And the worst case
> effect you can engineer by trying is to display some slightly odd symbols
If it would only display some slightly odd symbols I wouldn't complain.
The problem is that the second byte is interpreted as a control code.
Is there any trick to get the shell working again in this situation?
The cursor hangs, and I've not yet found a trick to do anything in this
xterm again (except for killing it from another xterm).
> (And incidentially since the Linux fs has been defined to be utf-8 for
> naming for many years you'll find the same problem using "ls")
No, "ls" can handle it perfectly:
# echo $LANG
C
# ls
??rsted
#
Or:
$ echo $LANG
en_US
$ ls
Ã?rsted
$
Different from the lsmod example, the cursor doesn't hang and the shell
is usable after this command.
The difference is that "ls" expects and handles such issues while
"lsmod" (and most likely also other userspace working with kernel
output) does not yet handle it resulting in problems if bytes are
wrongly interpreted as control codes.
> > - get module-init-tools fixed and
> > - document that 2.6.23 (or whichever will be the first kernel to support
> > UTF-8 in MODULE_AUTHOR) will require updated module-init-tools.
>
> "Require" is a rather strong word for a print formatting issue specific
> to obscure setups.
See obove, it's not only "print formatting", it's "kills my shell".
> Alan
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Fri, Jun 08, 2007 at 01:21:52AM +0200, Adrian Bunk wrote:
>...
> I added a MODULE_AUTHOR("J. Ørsted <[EMAIL PROTECTED]>") into the "raw"
> module:
>
> # echo $LANG
> C
> # modinfo --version
> module-init-tools version 3.3-pre11
> # modinfo raw
> filename: /lib/modules/2.6.21.2/kernel/drivers/char/raw.ko
> author: J. Ã
> ^ the cursor hangs here
>...
If anyone's wondering what's happening:
The UTF-8 representation of the character Ø consists of the two bytes
0xC3 0x98
In the ISO/IEC 8859 encodings where every character is represented by
one bytes this corresponds to two characters:
In ISO/IEC 8859-1 the byte 0xC3 represents the character à resulting in
the (harmless) display of this wrong character.
But in all the ISO/IEC 8859 encodings, the byte 0x98 is the
_control code_ "Start of String".
Therefore, if we want start using UTF-8 anywhere into the kernel, we
must ensure that all applications correctly convert all characters
if running in a non-UTF-8 environment.
I'm not sure that's worth the hassle.
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
> I added a MODULE_AUTHOR("J. Ørsted <[EMAIL PROTECTED]>") into the "raw"
> module:
>
> # echo $LANG
> C
> # modinfo --version
> module-init-tools version 3.3-pre11
> # modinfo raw
> filename: /lib/modules/2.6.21.2/kernel/drivers/char/raw.ko
> author: J. Ã
> ^ the cursor hangs here
"Mummy if I deliberately shoot myself in the head it hurts"
Distro's don't ship in C locale and haven't for years. And the worst case
effect you can engineer by trying is to display some slightly odd symbols
(And incidentially since the Linux fs has been defined to be utf-8 for
naming for many years you'll find the same problem using "ls")
> - get module-init-tools fixed and
> - document that 2.6.23 (or whichever will be the first kernel to support
> UTF-8 in MODULE_AUTHOR) will require updated module-init-tools.
"Require" is a rather strong word for a print formatting issue specific
to obscure setups.
Alan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Thu, Jun 07, 2007 at 11:22:48PM +0100, Alan Cox wrote:
> On Thu, 7 Jun 2007 21:34:13 +0200
> Adrian Bunk <[EMAIL PROTECTED]> wrote:
>
> > On Thu, Jun 07, 2007 at 04:28:20PM +0200, Jan Engelhardt wrote:
> > >
> > > On Jun 6 2007 11:05, Jesper Juhl wrote:
> > > >
> > > > - Source files should be 7bit ASCII
> > >
> > > Nah. Think of
> > >
> > > MODULE_AUTHOR("J. Ørsted <[EMAIL PROTECTED]>");
> > >...
> >
> > NO!
> >
> > Code must be 7bit ASCII.
> > This includes everything that gets into the kernel image.
>
> Disagree Adrian
>
> For quoted strings you want to include Unicode where appropriate, and the
> names of people happens to be highly appropriate. Trashing non US names
> is just rude, and in many cases extremely problematic because losing
> accent marks totally changes the meaning of the word and the
> pronunciation of the name.
>
> Now anyone who puts UTF-8 in the driver name or module options should get
> a lot of NAKs but putting it in the Author name is precisely where it is
> appropriate and correct. I suspect Author names are almost the only case
> where this is appropriate and/or neccessary.
I added a MODULE_AUTHOR("J. Ørsted <[EMAIL PROTECTED]>") into the "raw"
module:
# echo $LANG
C
# modinfo --version
module-init-tools version 3.3-pre11
# modinfo raw
filename: /lib/modules/2.6.21.2/kernel/drivers/char/raw.ko
author: J. Ã
^ the cursor hangs here
So for implementing your proposal, we have to:
- get module-init-tools fixed and
- document that 2.6.23 (or whichever will be the first kernel to support
UTF-8 in MODULE_AUTHOR) will require updated module-init-tools.
Oh, and when you are anyway planning to break older userspace, can you
remove the obsolete "raw" driver at the same time? ;-)
> Alan
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Thu, 7 Jun 2007 21:34:13 +0200
Adrian Bunk <[EMAIL PROTECTED]> wrote:
> On Thu, Jun 07, 2007 at 04:28:20PM +0200, Jan Engelhardt wrote:
> >
> > On Jun 6 2007 11:05, Jesper Juhl wrote:
> > >
> > > - Source files should be 7bit ASCII
> >
> > Nah. Think of
> >
> > MODULE_AUTHOR("J. Ørsted <[EMAIL PROTECTED]>");
> >...
>
> NO!
>
> Code must be 7bit ASCII.
> This includes everything that gets into the kernel image.
Disagree Adrian
For quoted strings you want to include Unicode where appropriate, and the
names of people happens to be highly appropriate. Trashing non US names
is just rude, and in many cases extremely problematic because losing
accent marks totally changes the meaning of the word and the
pronunciation of the name.
Now anyone who puts UTF-8 in the driver name or module options should get
a lot of NAKs but putting it in the Author name is precisely where it is
appropriate and correct. I suspect Author names are almost the only case
where this is appropriate and/or neccessary.
Alan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Thu, 7 Jun 2007 21:32:29 +0200 Adrian Bunk <[EMAIL PROTECTED]> wrote: > On Wed, Jun 06, 2007 at 11:05:07AM +0200, Jesper Juhl wrote: > >... > > - Source files should be 7bit ASCII and Documentation/Kbuild > > files/etc should be UTF-8, test that the patch honors that and doesn't > > put something else in (cleanups that remove 8bit ASCII etc from a > > source file is OK though). > >... > > That's wrong: > Code must be 7bit ASCII. > Comments in source files can be UTF-8. Also there is no such thing as 8bit ASCII. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Thu, Jun 07, 2007 at 04:28:20PM +0200, Jan Engelhardt wrote:
>
> On Jun 6 2007 11:05, Jesper Juhl wrote:
> >
> > - Source files should be 7bit ASCII
>
> Nah. Think of
>
> MODULE_AUTHOR("J. Ørsted <[EMAIL PROTECTED]>");
>...
NO!
Code must be 7bit ASCII.
This includes everything that gets into the kernel image.
> Jan
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Wed, Jun 06, 2007 at 11:05:07AM +0200, Jesper Juhl wrote: >... > - Source files should be 7bit ASCII and Documentation/Kbuild > files/etc should be UTF-8, test that the patch honors that and doesn't > put something else in (cleanups that remove 8bit ASCII etc from a > source file is OK though). >... That's wrong: Code must be 7bit ASCII. Comments in source files can be UTF-8. cu Adrian -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Jun 7 2007 12:46, Andy Whitcroft wrote: > >Jesper Juhl wrote: >> On 04/06/07, Andy Whitcroft <[EMAIL PROTECTED]> wrote: >>> >>> This version brings a host of changes to cure false positives and >>> bugs detected on patches submitted to lkml and -mm. It also brings >>> a number of new tests in response to reviews, of particular note: >>> >> A chmod +x scripts/checkpatch.pl would be nice :) > >While git carries the permissions I am am pretty sure a straight patch >doesn't. Where did you pick up your copy from linus' tree or from the >-mm one? Linus's tree lacks the +x bit... Jan -- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On 07/06/07, Jan Engelhardt <[EMAIL PROTECTED]> wrote:
On Jun 6 2007 11:05, Jesper Juhl wrote:
>
> - Source files should be 7bit ASCII
Nah. Think of
MODULE_AUTHOR("J. Ørsted <[EMAIL PROTECTED]>");
That's true. I wrote that comment shortly after reading
http://lkml.org/lkml/2007/6/4/448 , but you are right, 7bit ASCII can
be too limiting at times... Hmmm...
> - Maybe warn about usage of float/double in source files?
Generally yes, maybe, but see arch/i386/kernel/cpu/bugs.c,
arch/i386/math-emu/. Generally there is nothing to it. I think the
feature to allow the kernel to use [i387] FP without manually
saving/restoring the FP stack has been added some time ago.
I know there are places where floats and doubles can be used safely,
but for those rare occasions wouldn't it make sense to have the script
warn and require the submitter to justify the use? After all, the
general rule is to not use floating point in the kernel, so such a
patch is suspicious.
> - 'return' is not a function, so warn about patches that think it is
> and use 'return(expr);' (this one is tricky since 'return (expr);' can
> be OK in some cases.
Now, if we could detect superfluous parentheses and branches,
that'd be cool ;-) there are too many if ((a < 5) || (b > 6)) around.
Yeah wouldn't it be cool :-) It might require a bit too much perl
magic to actually implement something sane, but I just threw every
idea that came into my mind into the mail, assuming Andy could sort
out the ones that were a little too crazy ;)
--
Jesper Juhl <[EMAIL PROTECTED]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Jun 6 2007 11:05, Jesper Juhl wrote:
>
> - Source files should be 7bit ASCII
Nah. Think of
MODULE_AUTHOR("J. Ørsted <[EMAIL PROTECTED]>");
> - Maybe warn about usage of float/double in source files?
Generally yes, maybe, but see arch/i386/kernel/cpu/bugs.c,
arch/i386/math-emu/. Generally there is nothing to it. I think the
feature to allow the kernel to use [i387] FP without manually
saving/restoring the FP stack has been added some time ago.
> - 'return' is not a function, so warn about patches that think it is
> and use 'return(expr);' (this one is tricky since 'return (expr);' can
> be OK in some cases.
Now, if we could detect superfluous parentheses and branches,
that'd be cool ;-) there are too many if ((a < 5) || (b > 6)) around.
Jan
--
Re: [PATCH] update checkpatch.pl to version 0.03
On 07/06/07, Andy Whitcroft <[EMAIL PROTECTED]> wrote: Jesper Juhl wrote: > On 04/06/07, Andy Whitcroft <[EMAIL PROTECTED]> wrote: >> >> This version brings a host of changes to cure false positives and >> bugs detected on patches submitted to lkml and -mm. It also brings >> a number of new tests in response to reviews, of particular note: >> > A chmod +x scripts/checkpatch.pl would be nice :) While git carries the permissions I am am pretty sure a straight patch doesn't. Where did you pick up your copy from linus' tree or from the -mm one? via 'git pull' from Linus' tree. -- Jesper Juhl <[EMAIL PROTECTED]> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please http://www.expita.com/nomime.html - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
Jesper Juhl wrote: > On 04/06/07, Andy Whitcroft <[EMAIL PROTECTED]> wrote: >> >> This version brings a host of changes to cure false positives and >> bugs detected on patches submitted to lkml and -mm. It also brings >> a number of new tests in response to reviews, of particular note: >> > A chmod +x scripts/checkpatch.pl would be nice :) While git carries the permissions I am am pretty sure a straight patch doesn't. Where did you pick up your copy from linus' tree or from the -mm one? -apw - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On 04/06/07, Andy Whitcroft <[EMAIL PROTECTED]> wrote: This version brings a host of changes to cure false positives and bugs detected on patches submitted to lkml and -mm. It also brings a number of new tests in response to reviews, of particular note: A chmod +x scripts/checkpatch.pl would be nice :) -- Jesper Juhl <[EMAIL PROTECTED]> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please http://www.expita.com/nomime.html - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On 04/06/07, Andy Whitcroft <[EMAIL PROTECTED]> wrote: This version brings a host of changes to cure false positives and bugs detected on patches submitted to lkml and -mm. It also brings a number of new tests in response to reviews, of particular note: I have a few ideas for additional checks you could add to that script: - Source files should be 7bit ASCII and Documentation/Kbuild files/etc should be UTF-8, test that the patch honors that and doesn't put something else in (cleanups that remove 8bit ASCII etc from a source file is OK though). - Check that nothing in the patch touches any file from Documentation/dontdiff - Check for an excessive number of blank lines - some people have a nasty tendency to put 3 or more blank lines between functions or between comments and next source line etc. - Check that all newlines added by the patch are "\n", not "\r", "\r\n" or "\n\r". - Check that, if the patch adds a new file, the new file ends with a newline. - Warn about usage of the "register" keyword. - Maybe warn about usage of float/double in source files? - 'return' is not a function, so warn about patches that think it is and use 'return(expr);' (this one is tricky since 'return (expr);' can be OK in some cases. That's all I can come up with at the moment. If more ideas pop up I'll let you know. Good work with that script btw. -- Jesper Juhl <[EMAIL PROTECTED]> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please http://www.expita.com/nomime.html - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
jschopp wrote: >> The original suggestion was to count them and only complain if there >> were "lots". I had thought though that the general consensus was that >> #ifdef in C files was pretty much frowned upon. I must admit to working >> to the "you must be able to justify all winges in the output" rather >> than expecting the result to be empty necessarily. > > I really think we should work towards, "if you see an error the odds are > overwhelming that we aren't wasting your time and you should fix this." > rather than, "every time you send a patch you will get a couple false > positives that you will have to think about and justify to yourself, > slowing down your sending the patch out and making more work for you". > I'm not saying we have to have 100% accuracy, but I want it well in the > 90s. Yeah I tend to agree and most of my work to date has been squashing false positives. > Now back to the ifdef's. I don't think we can really even say a lot or > a little is broken (short patch can do 1 ifdef that is stupid, long > patch could do several that are good). I think we'll either have to let > that one go or put it under a non-default flag.We do still have > humans reviewing code who can make judgement calls like if you #ifdefs > are good or not. > > What we can check for is #if 0 code. I hate that one. For now I've disabled this one. Put it in the freezer with the StudlyCaps one. I like the idea of checking for #if 0 as that is very likely bogus in a patch meant for inclusion. >> We've not talked about how the tool might grow. My thought was that >> soon we would enable the robot replies on linux-mm (say) and use the >> feedback from that to tune what we do and do not report. I have been >> pushing all of the contributions to -mm for sometime through it and >> cirtainly the #ifdef one once of the more common ones (other than white >> space dammage and >80 character lines). > > If you have reasonably good SPAM filters on your list and make the robot > so it is very good about only picking up mail that really are patches > then this could be a very good idea. Just be careful. I could send out > a bunch of mail with fake headers saying it was from say Andy Whitcroft, > then the robot replies and you got a bunch of junk mail. All very good points. It does only reply to emails which are clearly unidiff patches and silently drops all else. But the potential for spam is worrysome ... will think on this some and see if we can come up with a safety net. > User feedback is really useful, both for us and for the user. Based on > user feedback from the very small number of users I had I tweaked a lot > of regular expressions, added some new checks, and removed some I could > never get right. I am getting a fair bit of feedback, most of it copied to Joel and Randy so at least that bit is working. -apw - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
Randy Dunlap wrote: > On Mon, 04 Jun 2007 21:08:07 +0200 Rene Herman wrote: > >> On 06/04/2007 09:08 PM, Andy Whitcroft wrote: >> >>> I guess line length and white space checks make sense some degree on >>> those files. I'll sort that out and I guess we'll have anohter version. >> Could you then also post the thing itself, and not just a patch against the >> previous version for us chickens too scared to run -mm? > > I'd like to see it put on the web in a fixed location. Yep. That makes lots of sense. Am trying to arrange a fixed location. Failing that I'll have to shove it on my server and move it later. Will let you know the outcome. -apw - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
jschopp wrote: >> This version brings a host of changes to cure false positives and >> bugs detected on patches submitted to lkml and -mm. It also brings >> a number of new tests in response to reviews, of particular note: >> >> - catch use of volatile >> - allow deprecated functions to be listed in >> feature-removal-schedule.txt >> - warn about #ifdef's in c files > > > I think the design philosophy of the style checker should be to err on > the side of being quiet. It shouldn't report things that aren't > problems. There are plenty of valid uses of #ifdefs in c files. > #ifdefs may be abused often. If we start bothering every author that > uses #ifdefs with an annoying note it detracts from the usefulness of > our tool. > > If we really want to complain about #ifdefs we should add a flag to the > script so it isn't a default. -potential or something. We could put > all the "this often is an error" type warnings under it. > > The rest of the patch looks fine. For now then we'll put the ifdef checks on ice until we get a better idea of the "rules" if we ever do. -apw - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Mon, Jun 04, 2007 at 10:46:24AM +0100, Andy Whitcroft wrote:
>
> This version brings a host of changes to cure false positives and
> bugs detected on patches submitted to lkml and -mm. It also brings
> a number of new tests in response to reviews, of particular note:
>
> - catch use of volatile
It will warn on "asm volatile (" which it shouldn't.
> - report on architecture specific defines being used
We use architecture specific defines to distinguish between 32 bit and
64 bit code in user space visible header files, since we cannot use
CONFIG_64BIT. So this will give us false positives as well.
Maybe don't warn for header files in include/asm-* ?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Mon, 04 Jun 2007 21:08:07 +0200 Rene Herman wrote: > On 06/04/2007 09:08 PM, Andy Whitcroft wrote: > > > I guess line length and white space checks make sense some degree on > > those files. I'll sort that out and I guess we'll have anohter version. > > Could you then also post the thing itself, and not just a patch against the > previous version for us chickens too scared to run -mm? I'd like to see it put on the web in a fixed location. --- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On 06/04/2007 09:08 PM, Andy Whitcroft wrote: I guess line length and white space checks make sense some degree on those files. I'll sort that out and I guess we'll have anohter version. Could you then also post the thing itself, and not just a patch against the previous version for us chickens too scared to run -mm? Rene. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
Andrew Morton wrote: > On Mon, 04 Jun 2007 10:46:24 +0100 > Andy Whitcroft <[EMAIL PROTECTED]> wrote: > >> This version brings a host of changes to cure false positives and >> bugs detected on patches submitted to lkml and -mm. It also brings >> a number of new tests in response to reviews, of particular note: >> >> - catch use of volatile >> - allow deprecated functions to be listed in feature-removal-schedule.txt >> - warn about #ifdef's in c files >> - check that spinlock_t and struct mutex use is commented >> - report on architecture specific defines being used >> - report memory barriers without an associated comment > > Oy. I ran checkpatch.pl across this patch and it failed to report upon the > new trailing whitespace which you just tried to add. Herm, I guess thats cause its a .pl and therefore exempt from most of the checks. I guess line length and white space checks make sense some degree on those files. I'll sort that out and I guess we'll have anohter version. Sounds like the #ifdef checks are too much anyhow. -apw - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Mon, 04 Jun 2007 10:46:24 +0100 Andy Whitcroft <[EMAIL PROTECTED]> wrote: > This version brings a host of changes to cure false positives and > bugs detected on patches submitted to lkml and -mm. It also brings > a number of new tests in response to reviews, of particular note: > > - catch use of volatile > - allow deprecated functions to be listed in feature-removal-schedule.txt > - warn about #ifdef's in c files > - check that spinlock_t and struct mutex use is commented > - report on architecture specific defines being used > - report memory barriers without an associated comment Oy. I ran checkpatch.pl across this patch and it failed to report upon the new trailing whitespace which you just tried to add. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
The original suggestion was to count them and only complain if there were "lots". I had thought though that the general consensus was that #ifdef in C files was pretty much frowned upon. I must admit to working to the "you must be able to justify all winges in the output" rather than expecting the result to be empty necessarily. I really think we should work towards, "if you see an error the odds are overwhelming that we aren't wasting your time and you should fix this." rather than, "every time you send a patch you will get a couple false positives that you will have to think about and justify to yourself, slowing down your sending the patch out and making more work for you". I'm not saying we have to have 100% accuracy, but I want it well in the 90s. Now back to the ifdef's. I don't think we can really even say a lot or a little is broken (short patch can do 1 ifdef that is stupid, long patch could do several that are good). I think we'll either have to let that one go or put it under a non-default flag.We do still have humans reviewing code who can make judgement calls like if you #ifdefs are good or not. What we can check for is #if 0 code. I hate that one. We've not talked about how the tool might grow. My thought was that soon we would enable the robot replies on linux-mm (say) and use the feedback from that to tune what we do and do not report. I have been pushing all of the contributions to -mm for sometime through it and cirtainly the #ifdef one once of the more common ones (other than white space dammage and >80 character lines). If you have reasonably good SPAM filters on your list and make the robot so it is very good about only picking up mail that really are patches then this could be a very good idea. Just be careful. I could send out a bunch of mail with fake headers saying it was from say Andy Whitcroft, then the robot replies and you got a bunch of junk mail. User feedback is really useful, both for us and for the user. Based on user feedback from the very small number of users I had I tweaked a lot of regular expressions, added some new checks, and removed some I could never get right. -Joel - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
jschopp wrote: >> This version brings a host of changes to cure false positives and >> bugs detected on patches submitted to lkml and -mm. It also brings >> a number of new tests in response to reviews, of particular note: >> >> - catch use of volatile >> - allow deprecated functions to be listed in >> feature-removal-schedule.txt >> - warn about #ifdef's in c files > > > I think the design philosophy of the style checker should be to err on > the side of being quiet. It shouldn't report things that aren't > problems. There are plenty of valid uses of #ifdefs in c files. > #ifdefs may be abused often. If we start bothering every author that > uses #ifdefs with an annoying note it detracts from the usefulness of > our tool. > > If we really want to complain about #ifdefs we should add a flag to the > script so it isn't a default. -potential or something. We could put > all the "this often is an error" type warnings under it. The original suggestion was to count them and only complain if there were "lots". I had thought though that the general consensus was that #ifdef in C files was pretty much frowned upon. I must admit to working to the "you must be able to justify all winges in the output" rather than expecting the result to be empty necessarily. I am ambivalent on what gets reported as long as its generally useful. I as you say don't want to produce so much noise that it hides the useful output. We've not talked about how the tool might grow. My thought was that soon we would enable the robot replies on linux-mm (say) and use the feedback from that to tune what we do and do not report. I have been pushing all of the contributions to -mm for sometime through it and cirtainly the #ifdef one once of the more common ones (other than white space dammage and >80 character lines). -apw - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
On Jun 4 2007 10:46, Andy Whitcroft wrote:
>
> - catch use of volatile
Speaking of volatile, "register" is probably just as unwanted.
Then, "extern inline" is one thing to catch (does not happen
that often, but it does not cost too much either).
> - warn about #ifdef's in c files
Really? There are a lot of ifdefs in existing code, and it is
probably inevitable to add some in newer code ... overall
leading to more false positives and cluttering the output.
Or am I gone wrong somewhere?
>+
>+ } elsif (/^Funcs:\s+(.*\S)/) {
>+ for my $func (split(/[, ]+/, $1)) {
>+ push(@dep_functions, $func);
>+ }
for -> foreach, although it does not functionally change anything.
Yeah, Perl is funny, for(one arg) is actually foreach().
Quite confusing to for(three args).
>+sub line_stats {
^ = \n ?
>+ last if (scalar(@o) == scalar(@c));
Or last if $#o == $#c. Again, personal taste (=do it your way).
I do not think $#a is any cheaper than scalar(@a) anyway.
>+sub has_non_quoted {
>+ return ($_[0] =~ m{$_[1]} and $_[0] !~ m{\".*$_[1].*\"});
>+}
Safe? Or intended? Did you want to allow regexes?
Otherwise...
return $_[0] =~ /\Q$_[1]\E/ && $_[0] !~ /".*\Q$_[1]\E.*"/;
> if (!($line =~ /^\s*Signed-off-by:/)) {
^ ^^
=> if($line !~ /.../)
Gotta love perl. Perhaps one language where you'll always find a
way to circumvent any CodingStyle ever written :p
> #80 column limit
>- if (!($prevline=~/\/\*\*/) && length($lineforcounting) > 80) {
>+ if (!($prevline=~/\/\*\*/) && $length > 80) {
I say thee 79. But we had that more or less already.
>+ for my $ctx (@ctx) {
>- while ($remaining > 0 && scalar @opened > scalar
>@closed) {
>+ while ($remaining > 1 && scalar @opened > scalar
>@closed) {
Although scalar might bind as hard as sizeof in C, I suggest parentheses.
(Or $#)
while ($remaining > 1 && scalar(@opened) > scalar(@closed))
>+# warn about #ifdefs in C files
>+ if ($line =~ /^.#\s*if(|n)def/ && ($realfile =~ /\.c$/)) {
save a capture operation: /^.#\s*ifn?def/
Jan
--
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] update checkpatch.pl to version 0.03
This version brings a host of changes to cure false positives and bugs detected on patches submitted to lkml and -mm. It also brings a number of new tests in response to reviews, of particular note: - catch use of volatile - allow deprecated functions to be listed in feature-removal-schedule.txt - warn about #ifdef's in c files I think the design philosophy of the style checker should be to err on the side of being quiet. It shouldn't report things that aren't problems. There are plenty of valid uses of #ifdefs in c files. #ifdefs may be abused often. If we start bothering every author that uses #ifdefs with an annoying note it detracts from the usefulness of our tool. If we really want to complain about #ifdefs we should add a flag to the script so it isn't a default. -potential or something. We could put all the "this often is an error" type warnings under it. The rest of the patch looks fine. -Joel - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] update checkpatch.pl to version 0.03
This version brings a host of changes to cure false positives and
bugs detected on patches submitted to lkml and -mm. It also brings
a number of new tests in response to reviews, of particular note:
- catch use of volatile
- allow deprecated functions to be listed in feature-removal-schedule.txt
- warn about #ifdef's in c files
- check that spinlock_t and struct mutex use is commented
- report on architecture specific defines being used
- report memory barriers without an associated comment
Signed-off-by: Andy Whitcroft <[EMAIL PROTECTED]>
---
Full changelog:
Andy Whitcroft (19):
catch use of volatile
convert other quoted string checks to common routine
alloc deprecated functions to be listed in feature-removal-schedule.txt
split out the line length and indent for each line
improve switch block handling
handle GNU diff context lines with no leading space
warn about #ifdef's in c files
tidy up tests for signed-off-by using raw mode
check that spinlock_t and struct mutex use is commented
syntax checks for open brace placement may drop off the bottom of hunk
report memory barriers without an associated comment
when a sign off is present but ugly do not report it missing
do not mistake bitfield definitions for indented labels
report on architecture specific defines being used
major update to the operator checks
prevent switch/if/while etc matching foo_switch
generify assignement in condition error message
introduce an operator context marker
Version: 0.03
---
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
old mode 100644
new mode 100755
index e216d49..9590bbb
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -8,7 +8,7 @@ use strict;
my $P = $0;
-my $V = '0.01';
+my $V = '0.03';
use Getopt::Long qw(:config no_auto_abbrev);
@@ -38,7 +38,8 @@ if ($tree && !top_of_kernel_tree()) {
exit(2);
}
-my @deprecated = ();
+my @dep_includes = ();
+my @dep_functions = ();
my $removal = 'Documentation/feature-removal-schedule.txt';
if ($tree && -f $removal) {
open(REMOVE, "<$removal") || die "$P: $removal: open failed - $!\n";
@@ -46,9 +47,14 @@ if ($tree && -f $removal) {
if (/^Files:\s+(.*\S)/) {
for my $file (split(/[, ]+/, $1)) {
if ($file =~ [EMAIL PROTECTED]/(.*)@) {
- push(@deprecated, $1);
+ push(@dep_includes, $1);
}
}
+
+ } elsif (/^Funcs:\s+(.*\S)/) {
+ for my $func (split(/[, ]+/, $1)) {
+ push(@dep_functions, $func);
+ }
}
}
}
@@ -99,6 +105,97 @@ sub expand_tabs {
return $res;
}
+sub line_stats {
+ my ($line) = @_;
+
+ # Drop the diff line leader and expand tabs
+ $line =~ s/^.//;
+ $line = expand_tabs($line);
+
+ # Pick the indent from the front of the line.
+ my ($white) = ($line =~ /^(\s*)/);
+
+ return (length($line), length($white));
+}
+
+sub ctx_block_get {
+ my ($linenr, $remain, $outer) = @_;
+ my $line;
+ my $start = $linenr - 1;
+ my $end = $linenr - 1 + $remain;
+ my $blk = '';
+ my @o;
+ my @c;
+ my @res = ();
+
+ for ($line = $start; $line < $end; $line++) {
+ $blk .= $lines[$line];
+
+ @o = ($blk =~ /\{/g);
+ @c = ($blk =~ /\}/g);
+
+ if (!$outer || (scalar(@o) - scalar(@c)) == 1) {
+ push(@res, $lines[$line]);
+ }
+
+ last if (scalar(@o) == scalar(@c));
+ }
+
+ return @res;
+}
+sub ctx_block_outer {
+ my ($linenr, $remain) = @_;
+
+ return ctx_block_get($linenr, $remain, 1);
+}
+sub ctx_block {
+ my ($linenr, $remain) = @_;
+
+ return ctx_block_get($linenr, $remain, 0);
+}
+
+sub ctx_locate_comment {
+ my ($first_line, $end_line) = @_;
+
+ # Catch a comment on the end of the line itself.
+ my ($current_comment) = ($lines[$end_line - 1] =~ [EMAIL
PROTECTED](/\*.*\*/)\s*$@);
+ return $current_comment if (defined $current_comment);
+
+ # Look through the context and try and figure out if there is a
+ # comment.
+ my $in_comment = 0;
+ $current_comment = '';
+ for (my $linenr = $first_line; $linenr < $end_line; $linenr++) {
+ my $line = $lines[$linenr - 1];
+ ##warn " $line\n";
+ if ($linenr == $first_line and $line =~ [EMAIL PROTECTED]@) {
+ $in_comment = 1;
+ }
+ if ($line =~ m@/\*@) {
+ $in_comment = 1;
+ }
+ if (!$in_comment && $current_comment ne '') {
+
