| Message ID | 1411491955-9131-1-git-send-email-rob.jones@codethink.co.uk |
|---|---|
| State | Not Applicable |
| Delegated to: | Pablo Neira |
| Headers | show |
On Tue, 2014-09-23 at 18:05 +0100, Rob Jones wrote: > Reduce boilerplate code by using __seq_open_private() instead of seq_open() > in xt_match_open() and xt_target_open(). > > Signed-off-by: Rob Jones <rob.jones@codethink.co.uk> > --- > > This patch uses an existing variant of seq_open() to reduce the kernel code > size. > > The only significant variation from the pre-existing code is the fact that > __seq_open_private() calls kzalloc() rather than kmalloc(), which could > conceivably have an impact on timing. > > This version 2 incorporates a minor initialisation simplification (resulting > from kzalloc() being used) requested by netfilter-devel@vger.kernel.org [...] > --- a/net/netfilter/x_tables.c > +++ b/net/netfilter/x_tables.c > @@ -1080,7 +1080,6 @@ static void *xt_mttg_seq_start(struct seq_file *seq, loff_t *pos, > struct nf_mttg_trav *trav = seq->private; > unsigned int j; > > - trav->class = MTTG_TRAV_INIT; > for (j = 0; j < *pos; ++j) > if (xt_mttg_seq_next(seq, NULL, NULL, is_target) == NULL) > return NULL; [...] I'm fairly sure this simplification is wrong, as xt_mttg_seq_start() is potentially called multiple times on the same file handle. Ben. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 23, 2014 at 06:46:27PM +0100, Ben Hutchings wrote: > On Tue, 2014-09-23 at 18:05 +0100, Rob Jones wrote: > > Reduce boilerplate code by using __seq_open_private() instead of seq_open() > > in xt_match_open() and xt_target_open(). > > > > Signed-off-by: Rob Jones <rob.jones@codethink.co.uk> > > --- > > > > This patch uses an existing variant of seq_open() to reduce the kernel code > > size. > > > > The only significant variation from the pre-existing code is the fact that > > __seq_open_private() calls kzalloc() rather than kmalloc(), which could > > conceivably have an impact on timing. > > > > This version 2 incorporates a minor initialisation simplification (resulting > > from kzalloc() being used) requested by netfilter-devel@vger.kernel.org > [...] > > --- a/net/netfilter/x_tables.c > > +++ b/net/netfilter/x_tables.c > > @@ -1080,7 +1080,6 @@ static void *xt_mttg_seq_start(struct seq_file *seq, loff_t *pos, > > struct nf_mttg_trav *trav = seq->private; > > unsigned int j; > > > > - trav->class = MTTG_TRAV_INIT; > > for (j = 0; j < *pos; ++j) > > if (xt_mttg_seq_next(seq, NULL, NULL, is_target) == NULL) > > return NULL; > [...] > > I'm fairly sure this simplification is wrong, as xt_mttg_seq_start() is > potentially called multiple times on the same file handle. Right. I'm going to take v1 of this patch instead, sorry for the inconvenience Rob. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23/09/14 22:40, Pablo Neira Ayuso wrote: > On Tue, Sep 23, 2014 at 06:46:27PM +0100, Ben Hutchings wrote: >> On Tue, 2014-09-23 at 18:05 +0100, Rob Jones wrote: >>> Reduce boilerplate code by using __seq_open_private() instead of seq_open() >>> in xt_match_open() and xt_target_open(). >>> >>> Signed-off-by: Rob Jones <rob.jones@codethink.co.uk> >>> --- >>> >>> This patch uses an existing variant of seq_open() to reduce the kernel code >>> size. >>> >>> The only significant variation from the pre-existing code is the fact that >>> __seq_open_private() calls kzalloc() rather than kmalloc(), which could >>> conceivably have an impact on timing. >>> >>> This version 2 incorporates a minor initialisation simplification (resulting >>> from kzalloc() being used) requested by netfilter-devel@vger.kernel.org >> [...] >>> --- a/net/netfilter/x_tables.c >>> +++ b/net/netfilter/x_tables.c >>> @@ -1080,7 +1080,6 @@ static void *xt_mttg_seq_start(struct seq_file *seq, loff_t *pos, >>> struct nf_mttg_trav *trav = seq->private; >>> unsigned int j; >>> >>> - trav->class = MTTG_TRAV_INIT; >>> for (j = 0; j < *pos; ++j) >>> if (xt_mttg_seq_next(seq, NULL, NULL, is_target) == NULL) >>> return NULL; >> [...] >> >> I'm fairly sure this simplification is wrong, as xt_mttg_seq_start() is >> potentially called multiple times on the same file handle. Well spotted Ben. > > Right. I'm going to take v1 of this patch instead, sorry for the > inconvenience Rob. No problem at all. It gave me something to do on the train home last night :-)
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c index 227aa11..393f17b 100644 --- a/net/netfilter/x_tables.c +++ b/net/netfilter/x_tables.c @@ -1080,7 +1080,6 @@ static void *xt_mttg_seq_start(struct seq_file *seq, loff_t *pos, struct nf_mttg_trav *trav = seq->private; unsigned int j; - trav->class = MTTG_TRAV_INIT; for (j = 0; j < *pos; ++j) if (xt_mttg_seq_next(seq, NULL, NULL, is_target) == NULL) return NULL; @@ -1137,22 +1136,11 @@ static const struct seq_operations xt_match_seq_ops = { static int xt_match_open(struct inode *inode, struct file *file) { - struct seq_file *seq; struct nf_mttg_trav *trav; - int ret; - - trav = kmalloc(sizeof(*trav), GFP_KERNEL); - if (trav == NULL) + trav = __seq_open_private(file, &xt_match_seq_ops, sizeof(*trav)); + if (!trav) return -ENOMEM; - ret = seq_open(file, &xt_match_seq_ops); - if (ret < 0) { - kfree(trav); - return ret; - } - - seq = file->private_data; - seq->private = trav; trav->nfproto = (unsigned long)PDE_DATA(inode); return 0; } @@ -1201,22 +1189,11 @@ static const struct seq_operations xt_target_seq_ops = { static int xt_target_open(struct inode *inode, struct file *file) { - struct seq_file *seq; struct nf_mttg_trav *trav; - int ret; - - trav = kmalloc(sizeof(*trav), GFP_KERNEL); - if (trav == NULL) + trav = __seq_open_private(file, &xt_target_seq_ops, sizeof(*trav)); + if (!trav) return -ENOMEM; - ret = seq_open(file, &xt_target_seq_ops); - if (ret < 0) { - kfree(trav); - return ret; - } - - seq = file->private_data; - seq->private = trav; trav->nfproto = (unsigned long)PDE_DATA(inode); return 0; }
Reduce boilerplate code by using __seq_open_private() instead of seq_open() in xt_match_open() and xt_target_open(). Signed-off-by: Rob Jones <rob.jones@codethink.co.uk> --- This patch uses an existing variant of seq_open() to reduce the kernel code size. The only significant variation from the pre-existing code is the fact that __seq_open_private() calls kzalloc() rather than kmalloc(), which could conceivably have an impact on timing. This version 2 incorporates a minor initialisation simplification (resulting from kzalloc() being used) requested by netfilter-devel@vger.kernel.org net/netfilter/x_tables.c | 31 ++++--------------------------- 1 file changed, 4 insertions(+), 27 deletions(-)