(resend) reuse xxx_fifo_fops for xxx_pipe_fops
PATCH 1/1 (resend) reuse xxx_fifo_fops for xxx_pipe_fops
Hi Andrew, Al,
I posted this patch a few months ago, but it apparently fell thru cracks. Here we go again.
I noticed that read/write/rdwr_pipe_fops are (1) const and
(2) exactly identical to xxx_fifo_fops, which are also const.
Attached patch #defines xxx_pipe_fops as aliases to xxx_fifo_fops. Size difference:
# size linux-2.6.25-rc6*/*/pipe.o text data bss dec hex filename 6534 144 0 6678 1a16 linux-2.6.25-rc6/fs/pipe.o 5862 144 0 6006 1776 linux-2.6.25-rc6-pt/fs/pipe.o
Run-tested on 2.6.26-rc8. Please apply.
--
vda
fs/pipe.c +3 -36
@@ -814,42 +814,9 @@ .fasync = pipe_rdwr_fasync, }; -static const struct file_operations read_pipe_fops = { - .llseek = no_llseek, - .read = do_sync_read, - .aio_read = pipe_read, - .write = bad_pipe_w, - .poll = pipe_poll, - .unlocked_ioctl = pipe_ioctl, - .open = pipe_read_open, - .release = pipe_read_release, - .fasync = pipe_read_fasync, -}; - -static const struct file_operations write_pipe_fops = { - .llseek = no_llseek, - .read = bad_pipe_r, - .write = do_sync_write, - .aio_write = pipe_write, - .poll = pipe_poll, - .unlocked_ioctl = pipe_ioctl, - .open = pipe_write_open, - .release = pipe_write_release, - .fasync = pipe_write_fasync, -}; - -static const struct file_operations rdwr_pipe_fops = { - .llseek = no_llseek, - .read = do_sync_read, - .aio_read = pipe_read, - .write = do_sync_write, - .aio_write = pipe_write, - .poll = pipe_poll, - .unlocked_ioctl = pipe_ioctl, - .open = pipe_rdwr_open, - .release = pipe_rdwr_release, - .fasync = pipe_rdwr_fasync, -}; +#define read_pipe_fops read_fifo_fops +#define write_pipe_fops write_fifo_fops +#define rdwr_pipe_fops rdwr_fifo_fopsstruct pipe_inode_info *alloc_pipe_info (struct inode *inode) {
Quoted 10 lines · Denys Vlasenko
Hi Andrew, Al,
I posted this patch a few months ago, but it apparently fell thru cracks. Here we go again.
I noticed that read/write/rdwr_pipe_fops are (1) const and
(2) exactly identical toxxx_fifo_fops, which are also const.Attached patch #defines
xxx_pipe_fopsas aliases toxxx_fifo_fops. Size difference:
Why do you need the #defines? Just use the _fifo_ name directly.
Quoted 11 lines · On Tue, 1 Jul 2008 11:03:02 +0200 Denys Vlasenko <[email protected]>…
Hi Andrew, Al,
I posted this patch a few months ago, but it apparently fell thru cracks. Here we go again.
I noticed that read/write/rdwr_pipe_fops are (1) const and
(2) exactly identical toxxx_fifo_fops, which are also const.Attached patch #defines
xxx_pipe_fopsas aliases toxxx_fifo_fops. Size difference:
Quoted 8 lines
# size linux-2.6.25-rc6*/*/pipe.o text data bss dec hex filename 6534 144 0 6678 1a16 linux-2.6.25-rc6/fs/pipe.o 5862 144 0 6006 1776 linux-2.6.25-rc6-pt/fs/pipe.oRun-tested on 2.6.26-rc8. Please apply.
Signed-off-by Denys Vlasenko <[email protected]>
<argh, an attachment. save-as, read, edit..>
--- linux-2.6.25-rc6.src/fs/pipe.c Sat Mar 22 23:00:34 2008
Quoted 37 lines
+++
linux-2.6.25-rc6.pipe/fs/pipe.cFri Mar 28 15:52:00 2008fs/pipe.c +0
-32@@ -814,42 +814,9 @@ .fasync = pipe_rdwr_fasync, }; -static const struct file_operations read_pipe_fops = { - .llseek = no_llseek, - .read = do_sync_read, - .aio_read = pipe_read, - .write = bad_pipe_w, - .poll = pipe_poll, - .unlocked_ioctl = pipe_ioctl, - .open = pipe_read_open, - .release = pipe_read_release, - .fasync = pipe_read_fasync, -}; - -static const struct file_operations write_pipe_fops = { - .llseek = no_llseek, - .read = bad_pipe_r, - .write = do_sync_write, - .aio_write = pipe_write, - .poll = pipe_poll, - .unlocked_ioctl = pipe_ioctl, - .open = pipe_write_open, - .release = pipe_write_release, - .fasync = pipe_write_fasync, -}; - -static const struct file_operations rdwr_pipe_fops = { - .llseek = no_llseek, - .read = do_sync_read, - .aio_read = pipe_read, - .write = do_sync_write, - .aio_write = pipe_write, - .poll = pipe_poll, - .unlocked_ioctl = pipe_ioctl,
fs/pipe.c line 822
Quoted 10 lines
fs/pipe.c +3
-4- .open = pipe_rdwr_open, - .release = pipe_rdwr_release, - .fasync = pipe_rdwr_fasync, -}; +#define read_pipe_fops read_fifo_fops +#define write_pipe_fops write_fifo_fops +#define rdwr_pipe_fops rdwr_fifo_fopsstruct pipe_inode_info *alloc_pipe_info (struct inode *inode) {
Well OK. But there's a risk that someone will go and modify read_fifo_fops without realising that they're also modifying read_pipe_fops.
So it'd be better to rename read_fifo_fops to (say) shared_read_fops then do
#define read_pipe_fops shared_read_fops
#define read_fifo_fops shared_read_fops
no?
Quoted 11 lines · Andrew Morton
Quoted 11 lines
I noticed that read/write/rdwr_pipe_fops are (1) const and
(2) exactly identical toxxx_fifo_fops, which are also const.Attached patch #defines
xxx_pipe_fopsas aliases toxxx_fifo_fops. Size difference:# size linux-2.6.25-rc6*/*/pipe.o text data bss dec hex filename 6534 144 0 6678 1a16 linux-2.6.25-rc6/fs/pipe.o 5862 144 0 6006 1776 linux-2.6.25-rc6-pt/fs/pipe.o
Quoted 5 lines
Quoted 3 lines
Run-tested on 2.6.26-rc8. Please apply.
Signed-off-by Denys Vlasenko <[email protected]><argh, an attachment. save-as, read, edit..>
Let's see how KMail will cope with inline cut-n-paste...
Quoted 11 lines
Quoted 11 lines
(quoted diff) +0
-11-static const struct file_operations rdwr_pipe_fops = { - .llseek = no_llseek, - .read = do_sync_read, - .aio_read = pipe_read, - .write = do_sync_write, - .aio_write = pipe_write, - .poll = pipe_poll, - .unlocked_ioctl = pipe_ioctl, - .open = pipe_rdwr_open, - .release = pipe_rdwr_release, - .fasync = pipe_rdwr_fasync,
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +3
-1-}; +#define read_pipe_fops read_fifo_fops +#define write_pipe_fops write_fifo_fops +#define rdwr_pipe_fops rdwr_fifo_fopsWell OK. But there's a risk that someone will go and modify
read_fifo_fopswithout realising that they're also modifyingread_pipe_fops.
Yes, it is possible.
Quoted 5 lines
So it'd be better to rename
read_fifo_fopsto (say)shared_read_fopsthen do#define
read_pipe_fopsshared_read_fops
#defineread_fifo_fopsshared_read_fops
I think since XXX_pipe_fops are only used in this file, just explaining this in the comment would be enough.
Please take a look at the version below.
--
vda
fs/pipe.c renamed from fs/pipe.c.org +5 -40
@@ -777,6 +777,8 @@/* * The file_operations structs are not static because they * are also used in linux/fs/fifo.c to do operations on FIFOs. +* +* Pipes reuse fifos' file_operations structs. */ const struct file_operations read_fifo_fops = { .llseek = no_llseek,
@@ -815,43 +817,6 @@ .fasync = pipe_rdwr_fasync, }; -static const struct file_operations read_pipe_fops = { - .llseek = no_llseek, - .read = do_sync_read, - .aio_read = pipe_read, - .write = bad_pipe_w, - .poll = pipe_poll, - .unlocked_ioctl = pipe_ioctl, - .open = pipe_read_open, - .release = pipe_read_release, - .fasync = pipe_read_fasync, -}; - -static const struct file_operations write_pipe_fops = { - .llseek = no_llseek, - .read = bad_pipe_r, - .write = do_sync_write, - .aio_write = pipe_write, - .poll = pipe_poll, - .unlocked_ioctl = pipe_ioctl, - .open = pipe_write_open, - .release = pipe_write_release, - .fasync = pipe_write_fasync, -}; - -static const struct file_operations rdwr_pipe_fops = { - .llseek = no_llseek, - .read = do_sync_read, - .aio_read = pipe_read, - .write = do_sync_write, - .aio_write = pipe_write, - .poll = pipe_poll, - .unlocked_ioctl = pipe_ioctl, - .open = pipe_rdwr_open, - .release = pipe_rdwr_release, - .fasync = pipe_rdwr_fasync, -}; -struct pipe_inode_info *alloc_pipe_info (struct inode *inode) {struct pipe_inode_info *pipe;
@@ -927,7 +892,7 @@ inode->i_pipe = pipe; pipe->readers = pipe->writers =1 ; - inode->i_fop = &rdwr_pipe_fops; + inode->i_fop = &rdwr_fifo_fops;/* * Mark the inode dirty from the very beginning,
@@ -978,7 +943,7 @@d_instantiate (dentry, inode); err = -ENFILE ; - f =alloc_file (pipe_mnt, dentry,FMODE_WRITE , &write_pipe_fops); + f =alloc_file (pipe_mnt, dentry,FMODE_WRITE , &write_fifo_fops);if (!f) goto err_dentry; f->f_mapping = inode->i_mapping;
@@ -1021,7 +986,7 @@ f->f_pos =0 ; f->f_flags =O_RDONLY ; - f->f_op = &read_pipe_fops; + f->f_op = &read_fifo_fops; f->f_mode =FMODE_READ ; f->f_version =0 ;
Quoted 2 lines · On Tue, 1 Jul 2008 12:03:50 +0200 Denys Vlasenko <[email protected]>…
I think since
XXX_pipe_fopsare only used in this file, just explaining this in the comment would be enough.
no, a comment is only needed when the code is unobvious. Make the code obvious and we don't need a comment.
As Christoph pointed out, open-coding shared_read_fops everywhere might make sense too. It'd make it harder to unshare them later on, but that's pretty improbable.
Quoted 11 lines · Andrew Morton
Quoted 2 lines · On Tue, 1 Jul 2008 12:03:50 +0200 Denys Vlasenko <[email protected]>…
I think since
XXX_pipe_fopsare only used in this file, just explaining this in the comment would be enough.no, a comment is only needed when the code is unobvious. Make the code obvious and we don't need a comment.
As Christoph pointed out, open-coding
shared_read_fopseverywhere might make sense too. It'd make it harder to unshare them later on, but that's pretty improbable.
Deny's latest patch doesn't have the #defines anymore and looks quite reasonable to me. If you really insist on magic naming I think shared is a very bad choice because it doesn't have any useful meaning in this context. If you insist on magic naming do _pipefifo_, but I think the current version with the comment is much better.
Quoted 11 lines · Andrew Morton
Quoted 2 lines · On Tue, 1 Jul 2008 12:03:50 +0200 Denys Vlasenko <[email protected]>…
I think since
XXX_pipe_fopsare only used in this file, just explaining this in the comment would be enough.no, a comment is only needed when the code is unobvious. Make the code obvious and we don't need a comment.
As Christoph pointed out, open-coding
shared_read_fopseverywhere might make sense too. It'd make it harder to unshare them later on, but that's pretty improbable.
Ok.
--
vda
fs/fifo.c renamed from fs.org/fifo.c +4 -4
@@ -57,7 +57,7 @@* POSIX.1 says that O_NONBLOCK means return with the FIFO * opened, even when there is no process writing the FIFO. */ - filp->f_op = &read_fifo_fops; + filp->f_op = &read_pipefifo_fops; pipe->r_counter++;if (pipe->readers++ ==0 )wake_up_partner (inode);
@@ -86,7 +86,7 @@if ((filp->f_flags &O_NONBLOCK ) && !pipe->readers) goto err; - filp->f_op = &write_fifo_fops; + filp->f_op = &write_pipefifo_fops; pipe->w_counter++;if (!pipe->writers++)wake_up_partner (inode);
@@ -105,7 +105,7 @@* This implementation will NEVER block on a O_RDWR open, since * the process can at least talk to itself. */ - filp->f_op = &rdwr_fifo_fops; + filp->f_op = &rdwr_pipefifo_fops; pipe->readers++; pipe->writers++;
@@ -151,5 +151,5 @@* depending on the access mode of the file... */ const struct file_operations def_fifo_fops = { - .open = fifo_open,/* will set read or write pipe_fops */ + .open = fifo_open,/* will set read_ or write_pipefifo_fops */ };
fs/pipe.c renamed from fs.org/pipe.c +8 -43
94 lines not on this page · raw message
Quoted 6 lines · Denys Vlasenko
Quoted 5 lines · Andrew Morton
Quoted 2 lines · On Tue, 1 Jul 2008 12:03:50 +0200 Denys Vlasenko <[email protected]>…
I think since
XXX_pipe_fopsare only used in this file, just explaining this in the comment would be enough.
Quoted 8 lines
Quoted 6 lines
no, a comment is only needed when the code is unobvious. Make the code obvious and we don't need a comment.
As Christoph pointed out, open-coding
shared_read_fopseverywhere might make sense too. It'd make it harder to unshare them later on, but that's pretty improbable.Ok.
And this one is even better [because it actually compiles :) ]. I forgot to change fs.h - XXX_pipefifo_fops live there.
--
vda
fs/fifo.c renamed from fs.org/fifo.c +4 -4
@@ -57,7 +57,7 @@* POSIX.1 says that O_NONBLOCK means return with the FIFO * opened, even when there is no process writing the FIFO. */ - filp->f_op = &read_fifo_fops; + filp->f_op = &read_pipefifo_fops; pipe->r_counter++;if (pipe->readers++ ==0 )wake_up_partner (inode);
@@ -86,7 +86,7 @@if ((filp->f_flags &O_NONBLOCK ) && !pipe->readers) goto err; - filp->f_op = &write_fifo_fops; + filp->f_op = &write_pipefifo_fops; pipe->w_counter++;if (!pipe->writers++)wake_up_partner (inode);
@@ -105,7 +105,7 @@* This implementation will NEVER block on a O_RDWR open, since * the process can at least talk to itself. */ - filp->f_op = &rdwr_fifo_fops; + filp->f_op = &rdwr_pipefifo_fops; pipe->readers++; pipe->writers++;
@@ -151,5 +151,5 @@* depending on the access mode of the file... */ const struct file_operations def_fifo_fops = { - .open = fifo_open,/* will set read or write pipe_fops */ + .open = fifo_open,/* will set read_ or write_pipefifo_fops */ };
fs/pipe.c renamed from fs.org/pipe.c +8 -43
94 lines not on this page · raw message
include/linux/fs.h renamed from include.org/linux/fs.h +3 -3
@@ -1687,9 +1687,9 @@extern void make_bad_inode (struct inode *);extern int is_bad_inode (struct inode *); -extern const struct file_operations read_fifo_fops; -extern const struct file_operations write_fifo_fops; -extern const struct file_operations rdwr_fifo_fops; +extern const struct file_operations read_pipefifo_fops; +extern const struct file_operations write_pipefifo_fops; +extern const struct file_operations rdwr_pipefifo_fops;extern int fs_may_remount_ro (struct super_block *);