Skip to contentSkip to conversation

Linux-2.5.27

PATCH In review Linus Torvalds 16 participants 41 messages Jul 20, 2002 – Jul 24, 2002

Subsystems (20)
Networking Network drivers VFS SCSI +16 more subsystems
lore
Share & tools
lore

Linus Torvalds

author
#0 lore raw

Ok, it's out there now, again there may well have been patches I missed, I concentrated on the fundamental ones (ie starting LSM merge, and most noticeably probably merging Rik's rmap patch through Andrew Morton along with other work by Andrew).

AGP split up and cleaned up, IDE patches 99-100 and 4GB FAT32 support.

And the inevitable USB updates, of course.

Linus

----

Summary of changes from v2.5.26 to v2.5.27
============================================

<[email protected]>:
o USB: patch to make USB_ZERO_PACKET work in ohci-hcd.c

<[email protected]>:
o USB: ov511 1.61 for 2.5.25

<[email protected]>:
o USB: usbserial.c fixup

Andrew Morton <[email protected]>:
o minimal rmap
o leave truncate's orphaned pages on the LRU
o avoid allocating pte_chains for unshared pages
o VM instrumentation
o O_DIRECT open check
o restore CHECK_EMERGENCY_SYNC. Again
o inline generic_writepages()
o alloc_pages cleanup
o direct_io mopup
o remove add_to_page_cache_unique()
o writeback scalability improvements
o readahead optimisations

David Brownell <[email protected]>:
o USB: usbnet queuing

Greg Kroah-Hartman <[email protected]>:
o agpgart: Split agpgart code into separate files
o agpgart: fix syntax error in the i8x0 file
o agpgart: renamed the agp files to make more sense
o agpgart: added agp prefix to the debug printk
o LSM: move the struct msg_msg and struct msg_queue definitions out of the msg.c file to the msg.h file
o LSM: move struct shmid_kernel out of ipc/shm.c to include/linux/shm.h
o agpgart: added "-agp" to the .c files that are for specific hardware types, based on mailing list comments
o USB: removed the usb-ohci driver, as it is no longer being used
o LSM: change BUS_ISA to CTL_BUS_ISA to prevent namespace collision with the input subsystem
o LSM: Add all of the new security/* files for basic task control
o LSM: Enable the security framework. This includes basic task control hooks
o LSM: for now, always set CONFIG_SECURITY_CAPABILITIES to y

Hirofumi Ogawa <[email protected]>:
o Add 4G-1 file support to FAT32

Linus Torvalds <[email protected]>:
o Remove "tristate" for CONFIG_SECURITY_CAPABILITIES, make it unconditional for now.

Martin Dalecki <[email protected]>:
o 2.5.26 IDE 99
o IDE 100

Neil Brown <[email protected]>:
o MD - Remove bdput calls from raid personalities
o MD - Remove dead consistancy checking code from multipath
o MD - Get multipath to use mempool
o MD - 27 - Remove state field from multipath mp_bh structure
o MD - Embed bio in mp_bh rather than separate allocation
o MD - Don't "analyze_sb" when creating new array
o MD - Use symbolic names for multipath (-4) and linear (-1)
o MD - Get rid of find_rdev_all
o MD - Rdev list cleanups
o MD - Pass the correct bdev to md_error
o MD - Move md_update_sb calls
o MD - Set desc_nr more sanely
o MD - Remove concept of 'spare' drive for multipath
o MD - Improve handling of spares in md
o MD - Add raid_disk field to rdev
o MD - Add in_sync flag to each rdev
o MD - Add "degraded" field to md device
o MD - when writing superblock, generate from mddev/rdev info
o MD - Don't maintain disc status in superblock
o MD - Remove old_dev field
o MD - nr_disks is gone from multipath/raid1
o MD - Remove number and raid_disk from personality arrays
o MD - Move persistent from superblock to mddev
o MD - Remove dependance on superblock
o MD - Remove the sb from the mddev
o MD - Change partition_name calls to bdev_partition_name were possible
o MD - Get rid of dev in rdev and use bdev exclusively

Oliver Neukum <[email protected]>:
o USB: lots of locking and other SMP race fixes

Rusty Russell <[email protected]>:
o drivers/usb/* designated initializer rework

Trond Myklebust <[email protected]>:
o Fix NFS locking bug
o Fix typo in net/sunrpc/xprt.c

#1 lore raw

PATCH 1/1 2.5.27 sysctl

This is making the sysctl code acutally be written in C.
It wasn't mostly due to georgeous ommitted size array "forward declarations". As a side effect it makes the table structure easier to deduce.

include/linux/sysctl.h +9 -9
diff -urN linux-2.5.27/include/linux/sysctl.h linux/include/linux/sysctl.h
@@ -126,7 +126,7 @@
 	KERN_S390_USER_DEBUG_LOGGING=51,  /* int: dumps of user faults */
 	KERN_CORE_USES_PID=52,		/* int: use core or core.%pid */
 	KERN_TAINTED=53,	/* int: various kernel tainted flags */
-	KERN_CADPID=54,		/* int: PID of the process to notify on CAD */
+	KERN_CADPID=54		/* int: PID of the process to notify on CAD */
 };
 
 

@@ -148,7 +148,7 @@
 	VM_DIRTY_SYNC=13,	/* dirty_sync_ratio */
 	VM_DIRTY_WB_CS=14,	/* dirty_writeback_centisecs */
 	VM_DIRTY_EXPIRE_CS=15,	/* dirty_expire_centisecs */
-	VM_NR_PDFLUSH_THREADS=16, /* nr_pdflush_threads */
+	VM_NR_PDFLUSH_THREADS=16 /* nr_pdflush_threads */
 };
 
 

@@ -225,7 +225,7 @@
 {
 	NET_UNIX_DESTROY_DELAY=1,
 	NET_UNIX_DELETE_DELAY=2,
-	NET_UNIX_MAX_DGRAM_QLEN=3,
+	NET_UNIX_MAX_DGRAM_QLEN=3
 };
 
 /* /proc/sys/net/ipv4 */

@@ -344,7 +344,7 @@
 	NET_IPV4_CONF_LOG_MARTIANS=11,
 	NET_IPV4_CONF_TAG=12,
 	NET_IPV4_CONF_ARPFILTER=13,
-	NET_IPV4_CONF_MEDIUM_ID=14,
+	NET_IPV4_CONF_MEDIUM_ID=14
 };
 
 /* /proc/sys/net/ipv6 */

@@ -553,7 +553,7 @@
 	FS_LEASES=13,	/* int: leases enabled */
 	FS_DIR_NOTIFY=14,	/* int: directory notification enabled */
 	FS_LEASE_TIME=15,	/* int: maximum time to wait for a lease break */
-	FS_DQSTATS=16,	/* disc quota usage statistics */
+	FS_DQSTATS=16	/* disc quota usage statistics */
 };
 
 /* /proc/sys/fs/quota/ */

@@ -565,7 +565,7 @@
 	FS_DQ_CACHE_HITS = 5,
 	FS_DQ_ALLOCATED = 6,
 	FS_DQ_FREE = 7,
-	FS_DQ_SYNCS = 8,
+	FS_DQ_SYNCS = 8
 };
 
 /* CTL_DEBUG names: */

@@ -619,12 +619,12 @@
 
 /* /proc/sys/dev/parport/parport n/devices/ */
 enum {
-	DEV_PARPORT_DEVICES_ACTIVE=-3,
+	DEV_PARPORT_DEVICES_ACTIVE=-3
 };
 
 /* /proc/sys/dev/parport/parport n/devices/device n */
 enum {
-	DEV_PARPORT_DEVICE_TIMESLICE=1,
+	DEV_PARPORT_DEVICE_TIMESLICE=1
 };
 
 /* /proc/sys/dev/mac_hid */

@@ -645,7 +645,7 @@
 	ABI_DEFHANDLER_LCALL7=3,/* default handler for procs using lcall7 */
 	ABI_DEFHANDLER_LIBCSO=4,/* default handler for an libc.so ELF interp */
 	ABI_TRACE=5,		/* tracing flags */
-	ABI_FAKE_UTSNAME=6,	/* fake target utsname information */
+	ABI_FAKE_UTSNAME=6	/* fake target utsname information */
 };
 
 #ifdef __KERNEL__
kernel/sysctl.c +47 -52

133 lines not on this page · raw message

net/sunrpc/sysctl.c +22 -23

55 lines not on this page · raw message

#2 lore raw

Re: [PATCH] 2.5.27 sysctl

Quoted 4 lines · Marcin Dalecki

This is making the sysctl code acutally be written in C.
It wasn't mostly due to georgeous ommitted size array "forward declarations". As a side effect it makes the table structure easier to deduce.

Please don't remove the trailing commas in the enums. they make adding to them much easier and are allowed by gcc (and maybe C99, I'm not sure).

#3 lore raw

Re: [PATCH] 2.5.27 sysctl

Quoted 11 lines · Christoph Hellwig
Quoted 4 lines · Marcin Dalecki

This is making the sysctl code acutally be written in C.
It wasn't mostly due to georgeous ommitted size array "forward declarations". As a side effect it makes the table structure easier to deduce.

Please don't remove the trailing commas in the enums. they make adding to them much easier and are allowed by gcc (and maybe C99, I'm not sure).

It's an GNU-ism. If you have any problem with "adding vales", just invent some dummy end-value. I have a problem with using -pedantic.

#4 lore raw

Re: [PATCH] 2.5.27 sysctl

Quoted 5 lines · Marcin Dalecki
Quoted 3 lines

Please don't remove the trailing commas in the enums. they make adding to them much easier and are allowed by gcc (and maybe C99, I'm not sure).

It's an GNU-ism.

So what?

The kernel is full of GNUisms, and this one is actually usefull.

Quoted 2 lines

If you have any problem with "adding vales", just
invent some dummy end-value. I have a problem with using -pedantic.

-pedantic barfs on named initializers, so you have to remove them first.

#5 lore raw

Re: [PATCH] 2.5.27 sysctl

Quoted 10 lines · Christoph Hellwig
Quoted 5 lines · Marcin Dalecki
Quoted 3 lines

Please don't remove the trailing commas in the enums. they make adding to them much easier and are allowed by gcc (and maybe C99, I'm not sure).

It's an GNU-ism.

So what?

So i did.

Quoted 2 lines

The kernel is full of GNUisms, and this one is actually usefull.

Its is not half as full as you may think.

#6 lore raw

Re: [PATCH] 2.5.27 sysctl

Quoted 3 lines · Marcin Dalecki
Quoted 1 line

The kernel is full of GNUisms, and this one is actually usefull.

Its is not half as full as you may think.

Trailing comma in enums has _exactly_ the same status as .foo = bar in structure initializers. Both appear in C99 and were compiler-specific extensions before that.

If -pedantic is unhappy about one but not another - take it with gcc folks, it's a bug in gcc.

Speaking of GNUisms, inline assembler is one and it's by far the worst obstacle to portability. Speaking of *REALLY* ugly stuff - may I point you to abuses of ##? Yes, it's legal C. No, using it is a Bad Idea(tm). And the actual uses of _that_ one in the tree can give you the second look at your breakfast - grep around and you'll see.

#7 lore raw

Re: [PATCH] 2.5.27 sysctl

Quoted 4 lines · Alexander Viro

On Mon, 22 Jul 2002, Marcin Dalecki wrote:

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

The kernel is full of GNUisms, and this one is actually usefull.

Its is not half as full as you may think.

Trailing comma in enums has _exactly_ the same status as .foo = bar in structure initializers. Both appear in C99 and were compiler-specific extensions before that.

Yes the C99 people did give in on this point :-). But only until recently. And really the only way to find out what the warinigs are about is to have a look at cparse.y...

Quoted 5 lines

If -pedantic is unhappy about one but not another - take it with gcc folks, it's a bug in gcc.

Speaking of GNUisms, inline assembler is one and it's by far the worst obstacle to portability.

Sure, but this extension is actually a justifyed one.

Quoted 4 lines

Speaking of *REALLY* ugly stuff - may I point
you to abuses of ##? Yes, it's legal C. No, using it is a Bad Idea(tm). And the actual uses of _that_ one in the tree can give you the second look at your breakfast - grep around and you'll see.

Da, kanieczna. Ja znaju: It's resulting in semi "self modifying" code. Sometimes usefull but not in 80% where they are actually used is the ({ ... }) extension for example as well. In most of the cases where its used we should simply rely on the fact that gcc got now much better at doing function inlining instead. See the spinlock patch for example. Best abuse example are the byte swapping macros...

But one quite justifyed example of usage is the
wait_event_interruptible(), where it's used to assert repeated condition
evaluation. However this puts the fact aside that wait_event_interruptible() isn't a polite abstraction in first place...

#8 lore raw

Re: [PATCH] 2.5.27 sysctl

In reply to Marcin Dalecki
Quoted 1 line · Marcin Dalecki

It's an GNU-ism.

Actually I've looked it up in my copy of the last C99 draft:

	[#5]  This  edition  replaces  the previous edition, ISO/IEC |
	9899:1990,   as   amended   and   corrected    by    ISO/IEC |
	9899/COR1:1994,    ISO/IEC   9899/COR2:1995,   and   ISO/IEC |
	9899/AMD1:1995.  Major changes  from  the  previous  edition |

include:

[some stuff snipped]

-- trailing comma allowed in enum declaration |

So please let it in.

#9 lore raw

Re: [PATCH] 2.5.27 sysctl

In reply to Marcin Dalecki
Quoted 9 lines · Marcin Dalecki
Quoted 8 lines · Christoph Hellwig
Quoted 4 lines · Marcin Dalecki

This is making the sysctl code acutally be written in C.
It wasn't mostly due to georgeous ommitted size array "forward declarations". As a side effect it makes the table structure easier to deduce.

Quoted 6 lines
Quoted 3 lines

Please don't remove the trailing commas in the enums. they make adding to them much easier and are allowed by gcc (and maybe C99, I'm not sure).

It's an GNU-ism. If you have any problem with "adding vales", just invent some dummy end-value. I have a problem with using -pedantic.

You seem to have it permanently engaged 8)

If you are upset about that GNUism why doesn't your patch fix the other GNU-isms in the same file ? Also the entire kernel is *full* of GNU C extensions.

#10 lore raw

Re: [PATCH] 2.5.27 sysctl

Quoted 21 lines · Alan Cox
Quoted 11 lines · Marcin Dalecki
Quoted 9 lines · Christoph Hellwig
Quoted 4 lines · Marcin Dalecki

This is making the sysctl code acutally be written in C.
It wasn't mostly due to georgeous ommitted size array "forward declarations". As a side effect it makes the table structure easier to deduce.

Quoted 6 lines
Quoted 3 lines

Please don't remove the trailing commas in the enums. they make adding to them much easier and are allowed by gcc (and maybe C99, I'm not sure).

It's an GNU-ism. If you have any problem with "adding vales", just invent some dummy end-value. I have a problem with using -pedantic.

Quoted 5 lines

You seem to have it permanently engaged 8)

If you are upset about that GNUism why doesn't your patch fix the other GNU-isms in the same file ? Also the entire kernel is *full* of GNU C extensions.

That's a common rumour. struct inits are going to go anyway. The rest is only about 30 ({ ... }) in inclue/linux.
Of course some of the GNU extensions are actually usefull. Trailing , at enum declarations make up for a nice shift reduce conflict expirence in yacc. (And perhaps slower compilation...)

#11 lore raw

RE: [PATCH] 2.5.27 sysctl

In reply to Marcin Dalecki
|-----Original Message-----
|From: [email protected] 
|[mailto:[email protected]] On Behalf Of Marcin Dalecki
|Sent: Monday, July 22, 2002 4:26 PM
|To: Christoph Hellwig
|Cc: [email protected]; Linus Torvalds; Kernel Mailing List
|Subject: Re: [PATCH] 2.5.27 sysctl
|
|
|Christoph Hellwig wrote:
|> On Mon, Jul 22, 2002 at 12:42:07PM +0200, Marcin Dalecki wrote:
|> 
|>>This is making the sysctl code acutally be written in C.
|>>It wasn't mostly due to georgeous ommitted size array "forward 
|>>declarations". As a side effect it makes the table structure 
|easier to 
|>>deduce.
|> 
|> 
|> Please don't remove the trailing commas in the enums.  they make 
|> adding to them much easier and are allowed by gcc (and maybe 
|C99, I'm 
|> not sure).
|
|It's an GNU-ism. If you have any problem with "adding vales", 
|just invent some dummy end-value. I have a problem with using 
|-pedantic.

Its not, ANSI C allows a trailing comma.

Balbir

**************************Disclaimer************************************

Information contained in this E-MAIL being proprietary to Wipro Limited is 'privileged' and 'confidential' and intended for use only by the individual
or entity to which it is addressed. You are notified that any use, copying
or dissemination of the information contained in the E-MAIL in any manner whatsoever is strictly prohibited.

***************************************************************************

#12 lore raw

Re: [PATCH] 2.5.27 sysctl

In reply to Marcin Dalecki
Quoted 5 lines · Marcin Dalecki
Quoted 3 lines

Please don't remove the trailing commas in the enums. they make adding to them much easier and are allowed by gcc (and maybe C99, I'm not sure).

It's an GNU-ism. If you have any problem with "adding vales", just invent some dummy end-value. I have a problem with using -pedantic.

If you feel like doing 'warnings patrol', then there are a bunch of more important regular warnings[1] that need fixing up without having to look through the pedantic output. Last I checked the pedantic stuff flagged a lot of bits that the fix ended up uglier than the warning (which 99.9% of people won't ever see anyway)

Dave

[1] Although more important would be stabilising IDE, but thats a sidenote.

Signature · 2 lines
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs
#13 lore raw

Re: [PATCH] 2.5.27 sysctl

In reply to Marcin Dalecki
Quoted 9 lines · Marcin Dalecki
Quoted 8 lines · Christoph Hellwig
Quoted 4 lines · Marcin Dalecki

This is making the sysctl code acutally be written in C.
It wasn't mostly due to georgeous ommitted size array "forward declarations". As a side effect it makes the table structure easier to deduce.

Quoted 7 lines
Quoted 3 lines

Please don't remove the trailing commas in the enums. they make adding to them much easier and are allowed by gcc (and maybe C99, I'm not sure).

It's an GNU-ism. If you have any problem with "adding vales", just invent some dummy end-value. I have a problem with using -pedantic.

Trailing commas in enums were NOT allowed before C99. C99 allows them, and gcc and some other compilers allowed this syntax before C99. Since the kernel is going to use the .field= (C99) syntax for structure initialisers anyway, I don't see any point in removing trailing commas. (Especially not in code I once wrote :-))

As an aside, the fact that gcc warns about them is probably a compiler

bug (http://gcc.gnu.org/c99status.html)

'In some places, -pedantic warnings don't take proper account of the standard version selected.'

Bart

Quoted 5 lines

-
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/

Signature · 2 lines
Bart Hartgers - TUE Eindhoven 
http://plasimo.phys.tue.nl/bart/contact.html
#14 lore raw

Re: [PATCH] 2.5.27 sysctl

In reply to Marcin Dalecki

Am Mon, 2002-07-22 um 12.42 schrieb Marcin Dalecki:

include/linux/sysctl.h line 130

Quoted 10 lines

diff -urN linux-2.5.27/include/linux/sysctl.h linux/include/linux/sysctl.h

include/linux/sysctl.h +1 -1
@@ -126,7 +126,7 @@
 	KERN_S390_USER_DEBUG_LOGGING=51,  /* int: dumps of user faults */
 	KERN_CORE_USES_PID=52,		/* int: use core or core.%pid */
 	KERN_TAINTED=53,	/* int: various kernel tainted flags */
-	KERN_CADPID=54,		/* int: PID of the process to notify on CAD */
+	KERN_CADPID=54		/* int: PID of the process to notify on CAD */
 };

Please don't do such changes, there's a reason for the trailing comma:
Making it easier to extend structures and enums.

Signature · 2 lines
Servus,
       Daniel
#15 lore raw

PATCH 1/1 2.5.27 devfs

In reply to Linus Torvalds

Kill two inlines which are notwhere used and which don't make sense in the case someone is not compiling devfs at all.

include/linux/devfs_fs_kernel.h +0 -10
diff -urN linux-2.5.27/include/linux/devfs_fs_kernel.h linux/include/linux/devfs_fs_kernel.h
@@ -286,16 +286,6 @@
     return;
 }
 
-static inline kdev_t devfs_alloc_devnum (char type)
-{
-    return NODEV;
-}
-
-static inline void devfs_dealloc_devnum (char type, kdev_t devnum)
-{
-    return;
-}
-
 static inline int devfs_alloc_unique_number (struct unique_numspace *space)
 {
     return -1;
#16 lore raw

Re: [PATCH] 2.5.27 devfs

Quoted 2 lines · Marcin Dalecki

Kill two inlines which are notwhere used and which don't make sense in the case someone is not compiling devfs at all.

Rejected. Linus, please don't apply this bogus patch. External patches and drivers rely on the inline stubs so that #ifdef CONFIG_DEVFS_FS isn't needed.

Martin, why are you bothering with this kind of false cleanup? These inline stubs don't take up any space in the object files, so why bother? Also, given that the stubs were carefully added in the first place, it suggests that there is a good reason for their presence.

Why didn't you stop and think it through before firing off a patch, or at least ask me if you couldn't see why? This "patch first, think/ask questions later" approach is disturbing.

				Regards,

					Richard....
Permanent: [email protected]
Current:   [email protected]
#17 lore raw

Re: [PATCH] 2.5.27 devfs

Quoted 9 lines · Richard Gooch
Quoted 2 lines · Marcin Dalecki

Kill two inlines which are notwhere used and which don't make sense in the case someone is not compiling devfs at all.

Rejected. Linus, please don't apply this bogus patch. External patches and drivers rely on the inline stubs so that #ifdef CONFIG_DEVFS_FS isn't needed.

Dare to actually *name* one of them?

Quoted 4 lines

Martin, why are you bothering with this kind of false cleanup? These inline stubs don't take up any space in the object files, so why bother? Also, given that the stubs were carefully added in the first place, it suggests that there is a good reason for their presence.

They where not "carefully added".

The interface you are exposing is bogous.
Look in md.c for one example why.

Last time I counted you provide at least three different ways of object allocations which play nasty games with major minor numbers in repeating code in drivers all scattered over the kernel.
cd-roms are treated special md.c is doing. And you are doing the
whole object management in a side step instead of embarcing the
normal structures holding already device information so you get
of course memmory management problems...

Quoted 3 lines

Why didn't you stop and think it through before firing off a patch, or at least ask me if you couldn't see why? This "patch first, think/ask questions later" approach is disturbing.

You didn't think doing devfs_fs_kernel.h. One simple sample from there:

devfs_get_maj_min(devfs_get_handle_from_inode((inode))

If I look at md.c which is using it... well better don't tell.

And the above of of course inside ({ })...

Everybody would expect the following to be only a single function:

extern devfs_handle_t devfs_get_handle (devfs_handle_t dir, const char extern devfs_handle_t devfs_find_handle (devfs_handle_t dir, const char

And it was of course too hard to unify ops and handle:

extern void *devfs_get_ops (devfs_handle_t de);
extern void devfs_put_ops (devfs_handle_t de);

You couldn't resist adding the redundant devfs_ prefix overall in the kernel:

extern devfs_register_chrdev (unsigned int major, const char *name,

                                  struct file_operations *fops);
extern int devfs_register_blkdev (unsigned int major, const char *name,
                                  struct block_device_operations *bdops);

extern int devfs_unregister_chrdev (unsigned int major, const char *name); extern int devfs_unregister_blkdev (unsigned int major, const char *name);

Three different allocators and deallocators for one single subsystem,
preserving the illusion that there is in linux a real difference between major and minor numbers...

extern int devfs_alloc_major (char type);
extern void devfs_dealloc_major (char type, int major); extern kdev_t devfs_alloc_devnum (char type);
extern void devfs_dealloc_devnum (char type, kdev_t devnum); extern int devfs_alloc_unique_number (struct unique_numspace *space); extern void devfs_dealloc_unique_number (struct unique_numspace *space,

                                         int number);

If flags are invalid -> add an invalid flag! instead of value return through pointer.

static inline int devfs_get_flags (devfs_handle_t de, unsigned int *flags)

{
    return 0;
}

And so on and so on.... Viro is simple right.

#18 lore raw

Re: [PATCH] 2.5.27 devfs

Quoted 7 lines · Marcin Dalecki
Quoted 6 lines · Richard Gooch
Quoted 2 lines · Marcin Dalecki

Kill two inlines which are notwhere used and which don't make sense in the case someone is not compiling devfs at all.

Quoted 5 lines
Quoted 3 lines

Rejected. Linus, please don't apply this bogus patch. External patches and drivers rely on the inline stubs so that #ifdef CONFIG_DEVFS_FS isn't needed.

Dare to actually *name* one of them?

[snip]

OK, that's enough. Martin, kindly stay the fsck away from that pile of garbage for a couple of weeks.

_All_ partition-related code is getting rewritten and the last thing we need right now is additional clutter in the neighborhood. And devfs_fs_kernel.h, shite as it is, qualifies.

#19 lore raw

Re: [PATCH] 2.5.27 devfs

Quoted 20 lines · Alexander Viro
Quoted 9 lines · Marcin Dalecki
Quoted 7 lines · Richard Gooch
Quoted 2 lines · Marcin Dalecki

Kill two inlines which are notwhere used and which don't make sense in the case someone is not compiling devfs at all.

Quoted 5 lines
Quoted 3 lines

Rejected. Linus, please don't apply this bogus patch. External patches and drivers rely on the inline stubs so that #ifdef CONFIG_DEVFS_FS isn't needed.

Dare to actually *name* one of them?

Quoted 8 lines

[snip]

OK, that's enough. Martin, kindly stay the fsck away from that pile of garbage for a couple of weeks.

_All_ partition-related code is getting rewritten and the last thing we need right now is additional clutter in the neighborhood. And devfs_fs_kernel.h, shite as it is, qualifies.

No problem as long as long somebody cares.
That part stuff needs treatment as well is obvious if one looks at the extensive allocation fallback chains I have in my small sand pille... Would you dare to keep the following botch in mind as well please:

/*
* Returns the (struct ata_device *) for a given device number. Return
* NULL if the given device number does not match any present drives. */
struct ata_device *get_info_ptr(kdev_t i_rdev)

{
	unsigned int major = major(i_rdev);
	int h;

	for (h = 0; h < MAX_HWIFS; ++h) {
		struct ata_channel *ch = &ide_hwifs[h];
		if (ch->present && major == ch->major) {
			int unit = DEVICE_NR(i_rdev);
			if (unit < MAX_DRIVES) {
				struct ata_device *drive = &ch->drives[unit];
				if (drive->present)
					return drive;
			}
			break;
		}
	}
	return NULL;
}

This get's feed to the revalidate method.

struct block_device_operations ide_fops[] = {{

        .owner =                THIS_MODULE,
        .open =                 ide_open,
        .release =              ide_release,
        .ioctl =                ata_ioctl,
        .check_media_change =   ide_check_media_change,
        .revalidate =           ata_revalidate

}};

and the following ide_xlate_1024(kdev_t i_rdev botch.

I would love to go the bdev way there too :-).
But then please keep in mind that the georgeous random number device is using the major number of a device all over the kernel... and it's feeding the following ugly global array:

void add_blkdev_randomness(int major)

{
        if (major >= MAX_BLKDEV)
                return;

        if (blkdev_timer_state[major] == 0) {
                rand_initialize_blkdev(major, GFP_ATOMIC);
                if (blkdev_timer_state[major] == 0)
                        return;
        }

        add_timer_randomness(blkdev_timer_state[major], 0x200+major);
}

Which should of course look more like:

void add_blkdev_randomness(struct block_device *ptr) {
add_timer_randomness(((unsigned long) ptr) %
SOME_REASONABLE_VALUE, 0x200);
}

Simple couldn't resist:

1. Enabled devfs... system printed far too long
incomprehensive device names and didn't reboot.

2. I disabled automatic devfs mount... system didn't find root part either.

The only single expirence in my life, where I thought that naming disks
C: D: E: Z: isn't the worst thing that can happen.

I went curious and looked there... and *cry*.

#20 lore raw

Re: [PATCH] 2.5.27 devfs

In reply to Marcin Dalecki
Quoted 7 lines · Marcin Dalecki
Quoted 6 lines · Richard Gooch
Quoted 2 lines · Marcin Dalecki

Kill two inlines which are notwhere used and which don't make sense in the case someone is not compiling devfs at all.

Quoted 5 lines
Quoted 3 lines

Rejected. Linus, please don't apply this bogus patch. External patches and drivers rely on the inline stubs so that #ifdef CONFIG_DEVFS_FS isn't needed.

Dare to actually *name* one of them?

Apart from my own sdmany patch, other people have contacted me about this feature and said they were making use of it. I don't bother tracking everyone who uses my code. I've got better things to do.

In any case, I don't *need* to justify it to you, particularly not when the compiler completely optimises the inline stubs away. There is *zero* benefit to removing them, and doing so only breaks external code.

Quoted 3 lines

You didn't think doing devfs_fs_kernel.h. One simple sample from there:

devfs_get_maj_min(devfs_get_handle_from_inode((inode))

You've managed to pick a function that I'm not thrilled about either. But it has been necessary.

Quoted 4 lines

Everybody would expect the following to be only a single function:

extern devfs_handle_t devfs_get_handle (devfs_handle_t dir, const char extern devfs_handle_t devfs_find_handle (devfs_handle_t dir, const char

devfs_get_handle() is the preferred interface. For compatibility reasons, devfs_find_handle() remains. However, if you look at the implementation for devfs_find_handle(), you'll notice that it's marked for removal. But I believe in stable interfaces, so I want to give people time to transition.

Quoted 4 lines

And it was of course too hard to unify ops and handle:

extern void *devfs_get_ops (devfs_handle_t de);
extern void devfs_put_ops (devfs_handle_t de);

Huh? They are different animals.

Quoted 9 lines

You couldn't resist adding the redundant devfs_ prefix overall in the kernel:

extern devfs_register_chrdev (unsigned int major, const char *name,

                                   struct file_operations *fops);
extern int devfs_register_blkdev (unsigned int major, const char *name,
                                   struct block_device_operations *bdops);

extern int devfs_unregister_chrdev (unsigned int major, const char *name); extern int devfs_unregister_blkdev (unsigned int major, const char *name);

These do subtly different things than the non "devfs_" versions.

Quoted 8 lines

Three different allocators and deallocators for one single subsystem, preserving the illusion that there is in linux a real difference between major and minor numbers...

extern int devfs_alloc_major (char type);
extern void devfs_dealloc_major (char type, int major); extern kdev_t devfs_alloc_devnum (char type); extern void devfs_dealloc_devnum (char type, kdev_t devnum);

Well, there *is* a difference between major and minor numbers. The two different interfaces service different driver requirements. Fortunately, one interface is built on top of the other.

Quoted 3 lines

extern int devfs_alloc_unique_number (struct unique_numspace *space); extern void devfs_dealloc_unique_number (struct unique_numspace *space,

                                          int number);

These are completely unrelated to device numbers, so there's no point even comparing them.

Quoted 7 lines

If flags are invalid -> add an invalid flag! instead of value return through pointer.

static inline int devfs_get_flags (devfs_handle_t de, unsigned int *flags)

{
     return 0;
}

That's a spurious objection. The return value indicates whether the entry was valid or not. That is quite separate from flag values. Mixing data types by overloading the return value is not a sensible approach.

				Regards,

					Richard....
Permanent: [email protected]
Current:   [email protected]
#21 lore raw

PATCH 1/1 2.5.27 sched

In reply to Linus Torvalds

- Don't use __wait_event_interruptible() use wait_event_interruptible() instead.
- Remove unused get_current_user() macro.

drivers/scsi/sg.c +11 -16
diff -urN linux-2.5.27/drivers/scsi/sg.c linux/drivers/scsi/sg.c
@@ -295,10 +295,9 @@
 	}
 	if (sdp->headfp && (flags & O_NONBLOCK))
             goto error_out;
-        res = 0; 
-	__wait_event_interruptible(sdp->o_excl_wait,
-	       ((sdp->headfp || sdp->exclude) ? 0 : (sdp->exclude = 1)),
-                                   res);
+
+	res = wait_event_interruptible(sdp->o_excl_wait,
+	       ((sdp->headfp || sdp->exclude) ? 0 : (sdp->exclude = 1)));
         if (res) {
             retval = res; /* -ERESTARTSYS because signal hit process */
 	    goto error_out;

@@ -307,15 +306,14 @@
     else if (sdp->exclude) { /* some other fd has an exclusive lock on dev */
         if (flags & O_NONBLOCK)
             goto error_out;
-        res = 0; 
-        __wait_event_interruptible(sdp->o_excl_wait, (! sdp->exclude), res);
+        res = wait_event_interruptible(sdp->o_excl_wait, (! sdp->exclude));
         if (res) {
             retval = res; /* -ERESTARTSYS because signal hit process */
 	    goto error_out;
         }
     }
     if (sdp->detached) {
-    	retval = -ENODEV;
+	retval = -ENODEV;
 	goto error_out;
     }
     if (! sdp->headfp) { /* no existing opens on this device */

@@ -400,9 +398,8 @@
         if (filp->f_flags & O_NONBLOCK)
             return -EAGAIN;
 	while (1) {
-	    res = 0;  /* following is a macro that beats race condition */
-	    __wait_event_interruptible(sfp->read_wait, (sdp->detached || 
-		    (srp = sg_get_rq_mark(sfp, req_pack_id))), res);
+	    res = wait_event_interruptible(sfp->read_wait, (sdp->detached ||
+		    (srp = sg_get_rq_mark(sfp, req_pack_id))));
 	    if (sdp->detached)
 		return -ENODEV;
 	    if (0 == res)

@@ -783,16 +780,14 @@
 		return -ENODEV;
 	    if(! scsi_block_when_processing_errors(sdp->device) )
 		return -ENXIO;
-	    result = verify_area(VERIFY_WRITE, (void *)arg, SZ_SG_IO_HDR);
-	    if (result) return result;
 	    result = sg_new_write(sfp, (const char *)arg, SZ_SG_IO_HDR,
 				  blocking, read_only, &srp);
-	    if (result < 0) return result;
+	    if (result < 0)
+		    return result;
 	    srp->sg_io_owned = 1;
 	    while (1) {
-		result = 0;  /* following macro to beat race condition */
-		__wait_event_interruptible(sfp->read_wait,
-		       (sdp->detached || sfp->closed || srp->done), result);
+		result = wait_event_interruptible(sfp->read_wait,
+		       (sdp->detached || sfp->closed || srp->done));
 		if (sdp->detached)
 		    return -ENODEV;
 		if (sfp->closed)
include/linux/sched.h +19 -28

65 lines not on this page · raw message

net/irda/af_irda.c +2 -3
diff -urN linux-2.5.27/net/irda/af_irda.c linux/net/irda/af_irda.c
@@ -2373,9 +2373,8 @@
 			add_timer(&(self->watchdog));
 
 			/* Wait for IR-LMP to call us back */
-			__wait_event_interruptible(self->query_wait,
-			   (self->cachediscovery!=NULL || self->errno==-ETIME),
-						   ret);
+			ret = wait_event_interruptible(self->query_wait,
+			   (self->cachediscovery!=NULL || self->errno==-ETIME));
 
 			/* If watchdog is still activated, kill it! */
 			if(timer_pending(&(self->watchdog)))
#22 lore raw

PATCH 1/1 2.5.27 smbiod

In reply to Linus Torvalds

Fix label at block end warning - don't write "assembler code".

fs/smbfs/smbiod.c +4 -6
diff -urN linux-2.5.27/fs/smbfs/smbiod.c linux/fs/smbfs/smbiod.c
@@ -233,14 +233,14 @@
 	int maxwork = 7;
 
 	if (server->state != CONN_VALID)
-		goto out;
+		return;
 
 	do {
 		result = smb_request_recv(server);
 		if (result < 0) {
 			server->state = CONN_INVALID;
 			smbiod_retry(server);
-			goto out;	/* reconnecting is slow */
+			return;	/* reconnecting is slow */
 		} else if (server->rstate == SMB_RECV_REQUEST)
 			smbiod_handle_request(server);
 	} while (result > 0 && maxwork-- > 0);

@@ -249,7 +249,7 @@
 	 * If there is more to read then we want to be sure to wake up again.
 	 */
 	if (server->state != CONN_VALID)
-		goto out;
+		return;
 	if (smb_recv_available(server) > 0)
 		set_bit(SMBIOD_DATA_READY, &smbiod_flags);
 

@@ -258,7 +258,7 @@
 		if (result < 0) {
 			server->state = CONN_INVALID;
 			smbiod_retry(server);
-			goto out;	/* reconnecting is slow */
+			return;	/* reconnecting is slow */
 		}
 	} while (result > 0);
 

@@ -267,8 +267,6 @@
 	 */
 	if (!list_empty(&server->xmitq))
 		set_bit(SMBIOD_DATA_READY, &smbiod_flags);
-
-out:
 }
 
 /*
#23 lore raw

Re: [PATCH] 2.5.27 smbiod

Quoted 1 line · Marcin Dalecki

Fix label at block end warning - don't write "assembler code".

...

Quoted 3 lines
fs/smbfs/smbiod.c +1 -1
 	if (server->state != CONN_VALID)
-		goto out;
+		return;

...

fs/smbfs/smbiod.c line 270

Quoted 3 lines
fs/smbfs/smbiod.c +0 -2
-
-out:
 }

Assembler? No, that would be Pascal. Many would argue that a return out of the middle is as ugly as a goto.

Maybe it doesn't matter, and maybe you did actually improve the code generation, but there's an obvious way to ditch the warning without changing so much. Note the semicolon:

out:;
}

#24 lore raw

PATCH 1/1 2.5.27 spinlock

In reply to Linus Torvalds

- Add missing _raw_write_trylock() definitions for the UP preemption case.

- Replace tons of georgeous macros for the UP preemption case with static inline functions. Much nicer to look at and more adequate then ({ xxxx }) in this case.

include/linux/spinlock.h +90 -55
diff -urN linux-2.5.27/include/linux/spinlock.h linux/include/linux/spinlock.h
@@ -37,10 +37,10 @@
 #define write_unlock_irqrestore(lock, flags)	do { write_unlock(lock); local_irq_restore(flags); } while (0)
 #define write_unlock_irq(lock)			do { write_unlock(lock); local_irq_enable();       } while (0)
 #define write_unlock_bh(lock)			do { write_unlock(lock); local_bh_enable();        } while (0)
-#define spin_trylock_bh(lock)			({ int __r; local_bh_disable();\
+#define spin_trylock_bh(lock)			do { int __r; local_bh_disable();\
 						__r = spin_trylock(lock);      \
 						if (!__r) local_bh_enable();   \
-						__r; })
+						__r; } while (0)
 
 /* Must define these before including other files, inline functions need them */
 

@@ -89,6 +89,7 @@
 #define _raw_spin_trylock(lock)	((void)(lock), 1)
 #define spin_unlock_wait(lock)	do { (void)(lock); } while(0)
 #define _raw_spin_unlock(lock)	do { (void)(lock); } while(0)
+#define _raw_write_trylock(lock) ((void)(lock), 1)
 
 /*
  * Read-write spinlocks, allowing multiple readers

@@ -124,64 +125,97 @@
 
 #define preempt_get_count() (current_thread_info()->preempt_count)
 
-#define preempt_disable() \
-do { \
-	++current_thread_info()->preempt_count; \
-	barrier(); \
-} while (0)
-
-#define preempt_enable_no_resched() \
-do { \
-	--current_thread_info()->preempt_count; \
-	barrier(); \
-} while (0)
-
-#define preempt_enable() \
-do { \
-	--current_thread_info()->preempt_count; \
-	barrier(); \
-	if (unlikely(test_thread_flag(TIF_NEED_RESCHED))) \
-		preempt_schedule(); \
-} while (0)
-
-#define preempt_check_resched() \
-do { \
-	if (unlikely(test_thread_flag(TIF_NEED_RESCHED))) \
-		preempt_schedule(); \
-} while (0)
-
-#define spin_lock(lock)	\
-do { \
-	preempt_disable(); \
-	_raw_spin_lock(lock); \
-} while(0)
-
-#define spin_trylock(lock)	({preempt_disable(); _raw_spin_trylock(lock) ? \
-				1 : ({preempt_enable(); 0;});})
-#define spin_unlock(lock) \
-do { \
-	_raw_spin_unlock(lock); \
-	preempt_enable(); \
-} while (0)
-
-#define spin_unlock_no_resched(lock) \
-do { \
-	_raw_spin_unlock(lock); \
-	preempt_enable_no_resched(); \
-} while (0)
-
-#define read_lock(lock)		({preempt_disable(); _raw_read_lock(lock);})
-#define read_unlock(lock)	({_raw_read_unlock(lock); preempt_enable();})
-#define write_lock(lock)	({preempt_disable(); _raw_write_lock(lock);})
-#define write_unlock(lock)	({_raw_write_unlock(lock); preempt_enable();})
-#define write_trylock(lock)	({preempt_disable();_raw_write_trylock(lock) ? \
-				1 : ({preempt_enable(); 0;});})
+static inline void preempt_disable(void)
+{
+	++current_thread_info()->preempt_count;
+	barrier();
+}
+
+static inline void preempt_enable_no_resched(void)
+{
+	--current_thread_info()->preempt_count;
+	barrier();
+}
+
+static inline void preempt_enable(void)
+{
+	--current_thread_info()->preempt_count;
+	barrier();
+	if (unlikely(test_thread_flag(TIF_NEED_RESCHED)))
+		preempt_schedule();
+}
+
+static inline void preempt_check_resched(void)
+{
+	if (unlikely(test_thread_flag(TIF_NEED_RESCHED)))
+		preempt_schedule();
+}
+
+static inline void spin_lock(spinlock_t *lock)
+{
+	preempt_disable();
+	_raw_spin_lock(lock);
+}
+
+static inline int spin_trylock(spinlock_t *lock)
+{
+	preempt_disable();
+	if (_raw_spin_trylock(lock))
+		return 1;
+	preempt_enable();
+	return 0;
+}
+
+static inline void spin_unlock(spinlock_t *lock)
+{
+	_raw_spin_unlock(lock);
+	preempt_enable();
+}
+
+static inline void spin_unlock_no_resched(spinlock_t *lock)
+{
+	_raw_spin_unlock(lock);
+	preempt_enable_no_resched();
+}
+
+static inline void read_lock(rwlock_t *lock)
+{
+	preempt_disable();
+	_raw_read_lock(lock);
+}
+
+static inline void read_unlock(rwlock_t *lock)
+{
+	_raw_read_unlock(lock);
+	preempt_enable();
+}
+
+static inline void write_lock(rwlock_t *lock)
+{
+	preempt_disable();
+	_raw_write_lock(lock);
+}
+
+static inline void write_unlock(rwlock_t *lock)
+{
+	_raw_write_unlock(lock);
+	preempt_enable();
+}
+
+static inline int write_trylock(rwlock_t *lock)
+{
+	preempt_disable();
+	if (_raw_write_trylock(lock))
+		return 1;
+	preempt_enable();
+	return 0;
+}
 
 #else
 
 #define preempt_get_count()		(0)
 #define preempt_disable()		do { } while (0)
-#define preempt_enable_no_resched()	do {} while(0)
+#define preempt_enable_no_resched()	do { } while (0)
 #define preempt_enable()		do { } while (0)
 #define preempt_check_resched()		do { } while (0)
 

@@ -195,6 +229,7 @@
 #define write_lock(lock)		_raw_write_lock(lock)
 #define write_unlock(lock)		_raw_write_unlock(lock)
 #define write_trylock(lock)		_raw_write_trylock(lock)
+
 #endif
 
 /* "lock on reference count zero" */
#25 lore raw

Re: [PATCH] 2.5.27 spinlock

Quoted 5 lines · On Mon, 22 Jul 2002 12:50:22 +0200 Marcin Dalecki <[email protected]> wrote:

- Add missing _raw_write_trylock() definitions for the UP preemption case.

- Replace tons of georgeous macros for the UP preemption case with static inline functions. Much nicer to look at and more adequate then ({ xxxx }) in this case.

Martin, this patch is wrong, obvious from casual reading:

include/linux/spinlock.h line 43

Quoted 6 lines
include/linux/spinlock.h +2 -2
-#define spin_trylock_bh(lock)			({ int __r; local_bh_disable();\
+#define spin_trylock_bh(lock)			do { int __r; local_bh_disable();\
 						__r = spin_trylock(lock);      \
 						if (!__r) local_bh_enable();   \
-						__r; })
+						__r; } while (0)

I know you're smarter than this Martin 8)

Rusty.
PS. If you want them re-xmitted to Linus, send to [email protected]...

Signature · 2 lines
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy
#26 lore raw

PATCH 1/1 2.5.27 wait

In reply to Linus Torvalds

- Struct initializers are in C now.
- Remove unused add_wait_queue_cond() macro.

include/linux/wait.h +5 -21
diff -urN linux-2.5.27/include/linux/wait.h linux/include/linux/wait.h
@@ -43,16 +43,16 @@
  */
 
 #define __WAITQUEUE_INITIALIZER(name, tsk) {				\
-	task:		tsk,						\
-	func:		default_wake_function,				\
-	task_list:	{ NULL, NULL } }
+	.task =		tsk,						\
+	.func =		default_wake_function,				\
+	.task_list =	{ NULL, NULL } }
 
 #define DECLARE_WAITQUEUE(name, tsk)					\
 	wait_queue_t name = __WAITQUEUE_INITIALIZER(name, tsk)
 
 #define __WAIT_QUEUE_HEAD_INITIALIZER(name) {				\
-	lock:		SPIN_LOCK_UNLOCKED,				\
-	task_list:	{ &(name).task_list, &(name).task_list } }
+	.lock =		SPIN_LOCK_UNLOCKED,				\
+	.task_list =	{ &(name).task_list, &(name).task_list } }
 
 #define DECLARE_WAIT_QUEUE_HEAD(name) \
 	wait_queue_head_t name = __WAIT_QUEUE_HEAD_INITIALIZER(name)

@@ -103,22 +103,6 @@
 	list_del(&old->task_list);
 }
 
-#define add_wait_queue_cond(q, wait, cond) \
-	({							\
-		unsigned long flags;				\
-		int _raced = 0;					\
-		spin_lock_irqsave(&(q)->lock, flags);	\
-		(wait)->flags = 0;				\
-		__add_wait_queue((q), (wait));			\
-		rmb();						\
-		if (!(cond)) {					\
-			_raced = 1;				\
-			__remove_wait_queue((q), (wait));	\
-		}						\
-		spin_lock_irqrestore(&(q)->lock, flags);	\
-		_raced;						\
-	})
-
 #endif /* __KERNEL__ */
 
 #endif
#27 lore raw

PATCH 1/1 2.5.27 enum

In reply to Linus Torvalds

- Fix a bunch of places where there are trailing "," at the end of enum declarations.

arch/i386/kernel/cpu/centaur.c +1 -1
diff -urN linux-2.5.27/arch/i386/kernel/cpu/centaur.c linux/arch/i386/kernel/cpu/centaur.c
@@ -267,7 +267,7 @@
 		DNA=1<<15,
 		ERETSTK=1<<16,
 		E2MMX=1<<19,
-		EAMD3D=1<<20,
+		EAMD3D=1<<20
 	};
 
 	char *name;
arch/i386/kernel/mtrr.c +1 -1
diff -urN linux-2.5.27/arch/i386/kernel/mtrr.c linux/arch/i386/kernel/mtrr.c
@@ -299,7 +299,7 @@
     MTRR_IF_INTEL,		/* Intel (P6) standard MTRRs */
     MTRR_IF_AMD_K6,		/* AMD pre-Athlon MTRRs */
     MTRR_IF_CYRIX_ARR,		/* Cyrix ARRs */
-    MTRR_IF_CENTAUR_MCR,	/* Centaur MCRs */
+    MTRR_IF_CENTAUR_MCR		/* Centaur MCRs */
 } mtrr_if = MTRR_IF_NONE;
 
 static __initdata char *mtrr_if_name[] = {
fs/proc/base.c +1 -1
diff -urN linux-2.5.27/fs/proc/base.c linux/fs/proc/base.c
@@ -54,7 +54,7 @@
 	PROC_PID_MAPS,
 	PROC_PID_CPU,
 	PROC_PID_MOUNTS,
-	PROC_PID_FD_DIR = 0x8000,	/* 0x8000-0xffff */
+	PROC_PID_FD_DIR = 0x8000	/* 0x8000-0xffff */
 };
 
 struct pid_entry {
include/linux/backing-dev.h +1 -1
diff -urN linux-2.5.27/include/linux/backing-dev.h linux/include/linux/backing-dev.h
@@ -13,7 +13,7 @@
  */
 enum bdi_state {
 	BDI_pdflush,		/* A pdflush thread is working this device */
-	BDI_unused,		/* Available bits start here */
+	BDI_unused		/* Available bits start here */
 };
 
 struct backing_dev_info {
include/linux/blkdev.h +2 -2
diff -urN linux-2.5.27/include/linux/blkdev.h linux/include/linux/blkdev.h
@@ -87,7 +87,7 @@
 
 	__REQ_SPECIAL,	/* driver suplied command */
 
-	__REQ_NR_BITS,	/* stops here */
+	__REQ_NR_BITS	/* stops here */
 };
 
 #define REQ_RW		(1 << __REQ_RW)

@@ -117,7 +117,7 @@
 
 enum blk_queue_state {
 	Queue_down,
-	Queue_up,
+	Queue_up
 };
 
 #define BLK_TAGS_PER_LONG	(sizeof(unsigned long) * 8)
include/linux/buffer_head.h +1 -1
diff -urN linux-2.5.27/include/linux/buffer_head.h linux/include/linux/buffer_head.h
@@ -24,7 +24,7 @@
 	BH_Async_Write,	/* Is under end_buffer_async_write I/O */
 
 	BH_Boundary,	/* Block is followed by a discontiguity */
-	BH_PrivateStart,/* not a state bit, but the first bit available
+	BH_PrivateStart /* not a state bit, but the first bit available
 			 * for private allocation by other entities
 			 */
 };
include/linux/device.h +2 -2
diff -urN linux-2.5.27/include/linux/device.h linux/include/linux/device.h
@@ -39,13 +39,13 @@
 	SUSPEND_NOTIFY,
 	SUSPEND_SAVE_STATE,
 	SUSPEND_DISABLE,
-	SUSPEND_POWER_DOWN,
+	SUSPEND_POWER_DOWN
 };
 
 enum {
 	RESUME_POWER_ON,
 	RESUME_RESTORE_STATE,
-	RESUME_ENABLE,
+	RESUME_ENABLE
 };
 
 struct device;
include/linux/genhd.h +1 -1
diff -urN linux-2.5.27/include/linux/genhd.h linux/include/linux/genhd.h
@@ -38,7 +38,7 @@
 /* Ours is not to wonder why.. */
 	BSD_PARTITION =	FREEBSD_PARTITION,
 	MINIX_PARTITION = 0x81,  /* Minix Partition ID */
-	UNIXWARE_PARTITION = 0x63,		/* Partition ID, same as */
+	UNIXWARE_PARTITION = 0x63		/* Partition ID, same as */
 						/* GNU_HURD and SCO Unix */
 };
include/linux/jbd.h +2 -3
diff -urN linux-2.5.27/include/linux/jbd.h linux/include/linux/jbd.h
@@ -226,13 +226,12 @@
 #endif		/* JBD_ASSERTIONS */
 
 enum jbd_state_bits {
-	BH_JBD			/* Has an attached ext3 journal_head */
-	  = BH_PrivateStart,	
+	BH_JBD = BH_PrivateStart, /* Has an attached ext3 journal_head */
 	BH_JWrite,		/* Being written to log (@@@ DEBUGGING) */
 	BH_Freed,		/* Has been freed (truncated) */
 	BH_Revoked,		/* Has been revoked from the log */
 	BH_RevokeValid,		/* Revoked flag is valid */
-	BH_JBDDirty,		/* Is dirty but journaled */
+	BH_JBDDirty		/* Is dirty but journaled */
 };
 
 BUFFER_FNS(JBD, jbd)
include/linux/netdevice.h +1 -1
diff -urN linux-2.5.27/include/linux/netdevice.h linux/include/linux/netdevice.h
@@ -688,7 +688,7 @@
 	NETIF_MSG_INTR		= 0x0200,
 	NETIF_MSG_TX_DONE	= 0x0400,
 	NETIF_MSG_RX_STATUS	= 0x0800,
-	NETIF_MSG_PKTDATA	= 0x1000,
+	NETIF_MSG_PKTDATA	= 0x1000
 };
 
 #define netif_msg_drv(p)	((p)->msg_enable & NETIF_MSG_DRV)
include/linux/netfilter_ipv4.h +1 -1
diff -urN linux-2.5.27/include/linux/netfilter_ipv4.h linux/include/linux/netfilter_ipv4.h
@@ -56,7 +56,7 @@
 	NF_IP_PRI_NAT_DST = -100,
 	NF_IP_PRI_FILTER = 0,
 	NF_IP_PRI_NAT_SRC = 100,
-	NF_IP_PRI_LAST = INT_MAX,
+	NF_IP_PRI_LAST = INT_MAX
 };
 
 /* Arguments for setsockopt SOL_IP: */
include/linux/personality.h +2 -2
diff -urN linux-2.5.27/include/linux/personality.h linux/include/linux/personality.h
@@ -33,7 +33,7 @@
 	ADDR_LIMIT_32BIT =	0x0800000,
 	SHORT_INODE =		0x1000000,
 	WHOLE_SECONDS =		0x2000000,
-	STICKY_TIMEOUTS	=	0x4000000,
+	STICKY_TIMEOUTS	=	0x4000000
 };
 
 /*

@@ -62,7 +62,7 @@
 	PER_RISCOS =		0x000c,
 	PER_SOLARIS =		0x000d | STICKY_TIMEOUTS,
 	PER_UW7 =		0x000e | STICKY_TIMEOUTS | MMAP_PAGE_ZERO,
-	PER_MASK =		0x00ff,
+	PER_MASK =		0x00ff
 };
include/linux/pkt_cls.h +5 -5
diff -urN linux-2.5.27/include/linux/pkt_cls.h linux/include/linux/pkt_cls.h
@@ -48,7 +48,7 @@
 	TCA_U32_LINK,
 	TCA_U32_DIVISOR,
 	TCA_U32_SEL,
-	TCA_U32_POLICE,
+	TCA_U32_POLICE
 };
 
 #define TCA_U32_MAX TCA_U32_POLICE

@@ -96,7 +96,7 @@
 	TCA_RSVP_DST,
 	TCA_RSVP_SRC,
 	TCA_RSVP_PINFO,
-	TCA_RSVP_POLICE,
+	TCA_RSVP_POLICE
 };
 
 #define TCA_RSVP_MAX TCA_RSVP_POLICE

@@ -126,7 +126,7 @@
 	TCA_ROUTE4_TO,
 	TCA_ROUTE4_FROM,
 	TCA_ROUTE4_IIF,
-	TCA_ROUTE4_POLICE,
+	TCA_ROUTE4_POLICE
 };
 
 #define TCA_ROUTE4_MAX TCA_ROUTE4_POLICE

@@ -138,7 +138,7 @@
 {
 	TCA_FW_UNSPEC,
 	TCA_FW_CLASSID,
-	TCA_FW_POLICE,
+	TCA_FW_POLICE
 };
 
 #define TCA_FW_MAX TCA_FW_POLICE

@@ -153,7 +153,7 @@
 	TCA_TCINDEX_SHIFT,
 	TCA_TCINDEX_FALL_THROUGH,
 	TCA_TCINDEX_CLASSID,
-	TCA_TCINDEX_POLICE,
+	TCA_TCINDEX_POLICE
 };
 
 #define TCA_TCINDEX_MAX        TCA_TCINDEX_POLICE
include/linux/pkt_sched.h +5 -5
diff -urN linux-2.5.27/include/linux/pkt_sched.h linux/include/linux/pkt_sched.h
@@ -128,7 +128,7 @@
 	TCA_CSZ_UNSPEC,
 	TCA_CSZ_PARMS,
 	TCA_CSZ_RTAB,
-	TCA_CSZ_PTAB,
+	TCA_CSZ_PTAB
 };
 
 /* TBF section */

@@ -147,7 +147,7 @@
 	TCA_TBF_UNSPEC,
 	TCA_TBF_PARMS,
 	TCA_TBF_RTAB,
-	TCA_TBF_PTAB,
+	TCA_TBF_PTAB
 };
 
 

@@ -181,7 +181,7 @@
 {
 	TCA_RED_UNSPEC,
 	TCA_RED_PARMS,
-	TCA_RED_STAB,
+	TCA_RED_STAB
 };
 
 struct tc_red_qopt

@@ -213,7 +213,7 @@
        TCA_GRED_UNSPEC,
        TCA_GRED_PARMS,
        TCA_GRED_STAB,
-       TCA_GRED_DPS,
+       TCA_GRED_DPS
 };
 
 #define TCA_SET_OFF TCA_GRED_PARMS

@@ -327,7 +327,7 @@
 	TCA_CBQ_OVL_STRATEGY,
 	TCA_CBQ_RATE,
 	TCA_CBQ_RTAB,
-	TCA_CBQ_POLICE,
+	TCA_CBQ_POLICE
 };
 
 #define TCA_CBQ_MAX	TCA_CBQ_POLICE
include/linux/pm.h +3 -3
diff -urN linux-2.5.27/include/linux/pm.h linux/include/linux/pm.h
@@ -45,7 +45,7 @@
 
 	/* base station management */
 	PM_EJECT,
-	PM_LOCK,
+	PM_LOCK
 };
 
 typedef int pm_request_t;

@@ -61,7 +61,7 @@
 	PM_USB_DEV,	    /* USB device */
 	PM_SCSI_DEV,	    /* SCSI device */
 	PM_ISA_DEV,	    /* ISA device */
-	PM_MTD_DEV,	    /* Memory Technology Device */
+	PM_MTD_DEV	    /* Memory Technology Device */
 };
 
 typedef int pm_dev_t;

@@ -77,7 +77,7 @@
 	PM_SYS_IRDA =	 0x41d00510, /* IRDA controller */
 	PM_SYS_FDC =	 0x41d00700, /* floppy controller */
 	PM_SYS_VGA =	 0x41d00900, /* VGA controller */
-	PM_SYS_PCMCIA =	 0x41d00e00, /* PCMCIA controller */
+	PM_SYS_PCMCIA =	 0x41d00e00  /* PCMCIA controller */
 };
 
 /*
include/linux/proc_fs.h +1 -1
diff -urN linux-2.5.27/include/linux/proc_fs.h linux/include/linux/proc_fs.h
@@ -21,7 +21,7 @@
  */
 
 enum {
-	PROC_ROOT_INO = 1,
+	PROC_ROOT_INO = 1
 };
 
 /* Finally, the dynamically allocatable proc entries are reserved: */
include/linux/root_dev.h +1 -1
diff -urN linux-2.5.27/include/linux/root_dev.h linux/include/linux/root_dev.h
@@ -11,7 +11,7 @@
 	Root_SDA1 = MKDEV(SCSI_DISK0_MAJOR, 1),
 	Root_SDA2 = MKDEV(SCSI_DISK0_MAJOR, 2),
 	Root_HDC1 = MKDEV(IDE1_MAJOR, 1),
-	Root_SR0 = MKDEV(SCSI_CDROM_MAJOR, 0),
+	Root_SR0 = MKDEV(SCSI_CDROM_MAJOR, 0)
 };
 
 extern dev_t ROOT_DEV;
include/linux/rtnetlink.h +4 -4
diff -urN linux-2.5.27/include/linux/rtnetlink.h linux/include/linux/rtnetlink.h
@@ -112,7 +112,7 @@
 	RTN_PROHIBIT,		/* Administratively prohibited	*/
 	RTN_THROW,		/* Not in this table		*/
 	RTN_NAT,		/* Translate this address	*/
-	RTN_XRESOLVE,		/* Use external resolver	*/
+	RTN_XRESOLVE		/* Use external resolver	*/
 };
 
 #define RTN_MAX RTN_XRESOLVE

@@ -278,7 +278,7 @@
 #define RTAX_CWND RTAX_CWND
 	RTAX_ADVMSS,
 #define RTAX_ADVMSS RTAX_ADVMSS
-	RTAX_REORDERING,
+	RTAX_REORDERING
 #define RTAX_REORDERING RTAX_REORDERING
 };
 

@@ -442,7 +442,7 @@
 #define IFLA_PRIORITY IFLA_PRIORITY
 	IFLA_MASTER,
 #define IFLA_MASTER IFLA_MASTER
-	IFLA_WIRELESS,		/* Wireless Extension event - see wireless.h */
+	IFLA_WIRELESS		/* Wireless Extension event - see wireless.h */
 #define IFLA_WIRELESS IFLA_WIRELESS
 };
 

@@ -503,7 +503,7 @@
 	TCA_OPTIONS,
 	TCA_STATS,
 	TCA_XSTATS,
-	TCA_RATE,
+	TCA_RATE
 };
 
 #define TCA_MAX TCA_RATE
include/linux/sunrpc/debug.h +1 -1
diff -urN linux-2.5.27/include/linux/sunrpc/debug.h linux/include/linux/sunrpc/debug.h
@@ -89,7 +89,7 @@
 	CTL_RPCDEBUG = 1,
 	CTL_NFSDEBUG,
 	CTL_NFSDDEBUG,
-	CTL_NLMDEBUG,
+	CTL_NLMDEBUG
 };
 
 #endif /* _LINUX_SUNRPC_DEBUG_H_ */
include/linux/sunrpc/msg_prot.h +1 -1
diff -urN linux-2.5.27/include/linux/sunrpc/msg_prot.h linux/include/linux/sunrpc/msg_prot.h
@@ -16,7 +16,7 @@
 	RPC_AUTH_UNIX  = 1,
 	RPC_AUTH_SHORT = 2,
 	RPC_AUTH_DES   = 3,
-	RPC_AUTH_KRB   = 4,
+	RPC_AUTH_KRB   = 4
 };
 
 enum rpc_msg_type {
include/linux/swap.h +1 -1
diff -urN linux-2.5.27/include/linux/swap.h linux/include/linux/swap.h
@@ -89,7 +89,7 @@
 enum {
 	SWP_USED	= (1 << 0),	/* is slot in swap_info[] used? */
 	SWP_WRITEOK	= (1 << 1),	/* ok to write to this swap?	*/
-	SWP_ACTIVE	= (SWP_USED | SWP_WRITEOK),
+	SWP_ACTIVE	= (SWP_USED | SWP_WRITEOK)
 };
 
 #define SWAP_CLUSTER_MAX 32
include/linux/tcp_diag.h +3 -3
diff -urN linux-2.5.27/include/linux/tcp_diag.h linux/include/linux/tcp_diag.h
@@ -34,7 +34,7 @@
 enum
 {
 	TCPDIAG_REQ_NONE,
-	TCPDIAG_REQ_BYTECODE,
+	TCPDIAG_REQ_BYTECODE
 };
 
 #define TCPDIAG_REQ_MAX TCPDIAG_REQ_BYTECODE

@@ -62,7 +62,7 @@
 	TCPDIAG_BC_D_LE,
 	TCPDIAG_BC_AUTO,
 	TCPDIAG_BC_S_COND,
-	TCPDIAG_BC_D_COND,
+	TCPDIAG_BC_D_COND
 };
 
 struct tcpdiag_hostcond

@@ -97,7 +97,7 @@
 {
 	TCPDIAG_NONE,
 	TCPDIAG_MEMINFO,
-	TCPDIAG_INFO,
+	TCPDIAG_INFO
 };
 
 #define TCPDIAG_MAX TCPDIAG_INFO
include/linux/writeback.h +1 -1
diff -urN linux-2.5.27/include/linux/writeback.h linux/include/linux/writeback.h
@@ -28,7 +28,7 @@
 	WB_SYNC_NONE =  0,	/* Don't wait on anything */
 	WB_SYNC_LAST =  1,	/* Wait on the last-written mapping */
 	WB_SYNC_ALL =   2,	/* Wait on every mapping */
-	WB_SYNC_HOLD =  3,	/* Hold the inode on sb_dirty for sys_sync() */
+	WB_SYNC_HOLD =  3	/* Hold the inode on sb_dirty for sys_sync() */
 };
 
 void writeback_unlocked_inodes(int *nr_to_write,
#28 lore raw

Re: [PATCH] 2.5.27 enum

Quoted 2 lines · Marcin Dalecki

- Fix a bunch of places where there are trailing "," at the end of enum declarations.

Please don't apply this. By leaving the trailing "," on enums, additional values can be added by merely inserting an additional + line in a patch, otherwise there are excess conflicts when multiple patches add values to the enum.

-ben

#29 lore raw

Re: [PATCH] 2.5.27 enum

From: Benjamin LaHaise <[email protected]>
Date: Mon, 22 Jul 2002 16:01:18 -0400

Please don't apply this. By leaving the trailing "," on enums, additional values can be added by merely inserting an additional + line in a patch, otherwise there are excess conflicts when multiple patches add values to the enum.

I totally agree.

What is the purpose of all of these zany patches? Are you going to remove the inline assembler from the whole tree too? :-)

#30 lore raw

Re: [PATCH] 2.5.27 enum

Quoted 3 lines · David S. Miller

I totally agree.

What is the purpose of all of these zany patches?

Harry: Well, a couple of things...
King: Correct, and one of those things is...?
Harry: Best not mentioned, really.
King: Right! And the other is fornication!

(with apologies to Richard Curtis and Rowan Atkinson...)

#31 lore raw

Re: [PATCH] 2.5.27 enum

In reply to David S. Miller

bcrl> Please don't apply this. By leaving the trailing "," on enums, bcrl> additional values can be added by merely inserting an additional bcrl> + line in a patch, otherwise there are excess conflicts when bcrl> multiple patches add values to the enum.

davem> I totally agree.

Is my memory hosed or was there some years back a patch that specifically *added* the trailing commas to the tree, for the express reasons Ben mentions above?

-JimC

#32 lore raw

Re: [PATCH] 2.5.27 enum

In reply to Benjamin LaHaise
Quoted 8 lines · Benjamin LaHaise
Quoted 2 lines · Marcin Dalecki

- Fix a bunch of places where there are trailing "," at the end of enum declarations.

Please don't apply this. By leaving the trailing "," on enums, additional values can be added by merely inserting an additional + line in a patch, otherwise there are excess conflicts when multiple patches add values to the enum.

Gratuitous 'cleanups' with no real redeeming feature also have another downside which a lot of people seem to overlook. They completely screws over anyone who also has a pending patch in that area if Linus applies it.

For most people this is five minutes work as they fix up by hand the single reject in one or two places. For people like myself keeping a large patchset, this is a lot of extra work for absolutely no gain. Two kernels later, someone adds a new sysctl which re-adds the , at the end anyway.

We have much bigger problems to fix than silly[1] things like this.

Dave

[1] Maybe silly is the wrong word to use, but I didn't want to use 'trivial' for fear of putting down the usefulness of Rusty's trivial patches.

Signature · 2 lines
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs
#33 lore raw

Re: [PATCH] 2.5.27 enum

Quoted 14 lines · Dave Jones
Quoted 8 lines · Benjamin LaHaise
Quoted 2 lines · Marcin Dalecki

- Fix a bunch of places where there are trailing "," at the end of enum declarations.

Please don't apply this. By leaving the trailing "," on enums, additional values can be added by merely inserting an additional + line in a patch, otherwise there are excess conflicts when multiple patches add values to the enum.

Gratuitous 'cleanups' with no real redeeming feature also have another downside which a lot of people seem to overlook. They completely screws over anyone who also has a pending patch in that area if Linus applies it.

Quoted 7 lines

For most people this is five minutes work as they fix up by hand the single reject in one or two places. For people like myself keeping a large patchset, this is a lot of extra work for absolutely no gain. Two kernels later, someone adds a new sysctl which re-adds the , at the end anyway.

We have much bigger problems to fix than silly[1] things like this.

Enabling -pedantic spotted me at least immediately at the bug in readv/writev fixed in the same series of kernel without resorting to LSB testing as Alan explained how he came across this botch. So it's not entierly futile.

But for the enum case I agree that GCC is hossed and simply shouldn't
warn about it if in C99 mode. Personally I was just still thinking at C89 level. Well after all I already fixed this in the GCC I use.

And no matter what - there is not much working in the sysctl area and
non sized array forward declarations are not a nice thing both: to read and to the compiler. Same applies to the gratitious macro or ({ }) overusages in the other patches. Inlined functions how the advantage of
1. stricter type checking.
2. Possibly faster compilation(if only included by files which really use them of course)

#34 lore raw

Re: [PATCH] 2.5.27 enum

In reply to Dave Jones
Quoted 14 lines · Dave Jones
Quoted 8 lines · Benjamin LaHaise
Quoted 2 lines · Marcin Dalecki

- Fix a bunch of places where there are trailing "," at the end of enum declarations.

Please don't apply this. By leaving the trailing "," on enums, additional values can be added by merely inserting an additional + line in a patch, otherwise there are excess conflicts when multiple patches add values to the enum.

Gratuitous 'cleanups' with no real redeeming feature also have another downside which a lot of people seem to overlook. They completely screws over anyone who also has a pending patch in that area if Linus applies it.

Quoted 5 lines

For most people this is five minutes work as they fix up by hand the single reject in one or two places. For people like myself keeping a large patchset, this is a lot of extra work for absolutely no gain. Two kernels later, someone adds a new sysctl which re-adds the , at the end anyway.

Now imagine my pain keeping in sync with IDE 2.5 or even reviewing IDE patches...

Regards
--
Bartlomiej

Quoted 17 lines

We have much bigger problems to fix than silly[1] things like this.

Dave

[1] Maybe silly is the wrong word to use, but I didn't want to use 'trivial' for fear of putting down the usefulness of Rusty's trivial patches.

--
| Dave Jones. http://www.codemonkey.org.uk
| SuSE Labs
-
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/

#35 lore raw

Re: [PATCH] 2.5.27 enum

In reply to Dave Jones
Quoted 5 lines · On Tue, 23 Jul 2002 14:27:04 +0200 Dave Jones <[email protected]> wrote:
Quoted 4 lines · Benjamin LaHaise
Quoted 2 lines · Marcin Dalecki

- Fix a bunch of places where there are trailing "," at the end of enum declarations.

Quoted 8 lines
Quoted 4 lines

Please don't apply this. By leaving the trailing "," on enums, additional values can be added by merely inserting an additional + line in a patch, otherwise there are excess conflicts when multiple patches add values to the enum.

Gratuitous 'cleanups' with no real redeeming feature also have another downside which a lot of people seem to overlook. They completely screws over anyone who also has a pending patch in that area if Linus applies it.

Yes. It particularly sucks on the "maintainerless" core code which is always in flux. This is also why I generally reject whitespace-cleanup patches, and originally rejected the "doesnt" patches (I got convinced by the pedants).

OTOH, 90% of kernel code is copied from elsewhere, so janitorial cleanups *are* worthwhile, as long as they are one-liners, or fix a real problem.

Rusty.

Signature · 2 lines
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy
#36 lore raw

Re: [PATCH] 2.5.27 enum

Quoted 6 lines · Rusty Russell

Yes. It particularly sucks on the "maintainerless" core code which is always in flux. This is also why I generally reject whitespace-cleanup patches, and originally rejected the "doesnt" patches (I got convinced by the pedants).

OTOH, 90% of kernel code is copied from elsewhere, so janitorial cleanups *are* worthwhile, as long as they are one-liners, or fix a real problem.

I agree in part. Take the initialiser patches you're currently carrying for example. Whilst they're more useful (and more likely) to get merged than the enum patches, they also have the annoying issue that anyone currently working on code near those gets shafted.

With large touching patches like these, the only way to not piss people off is to find out who's working on a particular area, and work with them. "Can you roll this into your current working tree, and push to Linus next time". Instead of just shovelling straight to Linus. (Note, you did seem to actually seem to do the right thing here FWICS.
have a gold star to go alongside your recent black one).

Dave

Signature · 2 lines
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs
#37 lore raw

PATCH 1/1 2.5.27 read_write

In reply to Linus Torvalds

- This is making the read_write.c C.

- It is fixing completely confused wild casting to 32 bits.

- Actually adding a comment explaining the obscure code, which is relying on integer arithmetics overflow.

fs/read_write.c +5 -5
diff -urN linux-2.5.27/fs/read_write.c linux/fs/read_write.c
@@ -307,11 +307,11 @@
 	ret = -EINVAL;
 	for (i = 0 ; i < count ; i++) {
 		size_t tmp = tot_len;
-		int len = iov[i].iov_len;
-		if (len < 0)
-			goto out;
-		(u32)tot_len += len;
-		if (tot_len < tmp || tot_len < (u32)len)
+		size_t len = iov[i].iov_len;
+
+		tot_len += len;
+		/* check for overflows */
+		if (tot_len < tmp || tot_len < len)
 			goto out;
 	}
#38 lore raw

Re: [PATCH] 2.5.27 read_write

Quoted 4 lines · Marcin Dalecki

- It is fixing completely confused wild casting to 32 bits.

- Actually adding a comment explaining the obscure code, which is relying on integer arithmetics overflow.

Better yet take the code from 2.4.19-rc3. The code you fixed up is still wrong. Sincie iov_len is not permitted to exceed 2Gb (SuS v3, found by the LSB test suite) the actual fix turns out to be even simpler and cleaner than the one you did

#39 lore raw

PATCH 1/1 2.5.27 read_write - take 2

Quoted 4 lines · Alan Cox
Quoted 2 lines · Marcin Dalecki

- It is fixing completely confused wild casting to 32 bits.

Quoted 8 lines
Quoted 2 lines

- Actually adding a comment explaining the obscure code, which is relying on integer arithmetics overflow.

Better yet take the code from 2.4.19-rc3. The code you fixed up is still wrong. Sincie iov_len is not permitted to exceed 2Gb (SuS v3, found by the LSB test suite) the actual fix turns out to be even simpler and cleaner than the one you did

You are right. It makes sese, since readv and writev are supposed to return ssize_t. Fixed patch version attached.

fs/read_write.c +8 -4
diff -urN linux-2.5.27/fs/read_write.c linux/fs/read_write.c
@@ -306,12 +306,16 @@
 	tot_len = 0;
 	ret = -EINVAL;
 	for (i = 0 ; i < count ; i++) {
-		size_t tmp = tot_len;
-		int len = iov[i].iov_len;
+		ssize_t tmp = tot_len;
+		ssize_t len = iov[i].iov_len;
+
+		/* check for SSIZE_MAX overflow */
 		if (len < 0)
 			goto out;
-		(u32)tot_len += len;
-		if (tot_len < tmp || tot_len < (u32)len)
+
+		tot_len += len;
+		/* check for overflows */
+		if (tot_len < tmp)
 			goto out;
 	}
#40 lore raw

Re: [PATCH] 2.5.27 read_write

In reply to Marcin Dalecki
Quoted 6 lines · Marcin Dalecki

- This is making the read_write.c C.

- It is fixing completely confused wild casting to 32 bits.

- Actually adding a comment explaining the obscure code, which is relying on integer arithmetics overflow.

This is the 2.4 patch. This passes the SuS LSB validation tests and gets 32/64bit behaviour as well as sign rules for iov_len elements right. At least I hope it does.

fs/read_write.c +13 -7
@@ -301,17 +301,23 @@
 	if (copy_from_user(iov, vector, count*sizeof(*vector)))
 		goto out;
 
-	/* BSD readv/writev returns EINVAL if one of the iov_len
-	   values < 0 or tot_len overflowed a 32-bit integer. -ink */
+	/*
+	 * Single unix specification:
+	 * We should -EINVAL if an element length is not >= 0 and fitting an ssize_t
+	 * The total length is fitting an ssize_t
+	 *
+	 * Be careful here because iov_len is a size_t not an ssize_t
+	 */
+	 
 	tot_len = 0;
 	ret = -EINVAL;
 	for (i = 0 ; i < count ; i++) {
-		size_t tmp = tot_len;
-		int len = iov[i].iov_len;
-		if (len < 0)
+		ssize_t tmp = tot_len;
+		ssize_t len = (ssize_t)iov[i].iov_len;
+		if (len < 0)	/* size_t not fitting an ssize_t .. */
 			goto out;
-		(u32)tot_len += len;
-		if (tot_len < tmp || tot_len < (u32)len)
+		tot_len += len;
+		if (tot_len < tmp) /* maths overflow on the ssize_t */
 			goto out;
 	}