| Message ID | 20250205131153.476278-10-ps.report@gmx.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Some pktgen fixes/improvments | expand |
On Wed, Feb 05, 2025 at 02:11:45PM +0100, Peter Seiderer wrote: > Align some variable declarations (in get_imix_entries and get_labels) to > the most common pattern (int instead of ssize_t/long) and adjust function > return value accordingly. > > Signed-off-by: Peter Seiderer <ps.report@gmx.net> Hi Peter, These comments are is true in general of this patchset, but particularly so in the case of this patch: * I think a more succinct subject would be nice. * I think the patch description should provide some reason _why_ the change is being made. Also, specifically relating to this patch, I wonder if it's scope ought to be extended. For example, the two callers of num_arg(), get_imix_entries() and pktgen_if_write() assign the return value of num_arg() to len, which is now an int in both functions. But num_args() returns a long. > --- > Changes v3 -> v4 > - new patch (factored out of patch 'net: pktgen: fix access outside of user > given buffer in pktgen_if_write()') > --- > net/core/pktgen.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > index 4f201a2db2dc..279910367ad4 100644 > --- a/net/core/pktgen.c > +++ b/net/core/pktgen.c > @@ -850,12 +850,11 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen) > * where each entry consists of size and weight delimited by commas. > * "size1,weight_1 size2,weight_2 ... size_n,weight_n" for example. > */ > -static ssize_t get_imix_entries(const char __user *buffer, > - struct pktgen_dev *pkt_dev) > +static int get_imix_entries(const char __user *buffer, > + struct pktgen_dev *pkt_dev) > { > - int i = 0; > - long len; > char c; > + int i = 0, len; Given it can be achieved with exactly the same lines changed, just in a different order, please arrange the local variable declarations in reverse xmas tree order - longest line to shortest. Likewise for the other hunk of this patch. And I believe there are also other cases in this patchset where this comment applied. The following tool can be useful: https://github.com/ecree-solarflare/xmastree ...
Hello Simon, On Thu, 6 Feb 2025 13:25:38 +0000, Simon Horman <horms@kernel.org> wrote: > On Wed, Feb 05, 2025 at 02:11:45PM +0100, Peter Seiderer wrote: > > Align some variable declarations (in get_imix_entries and get_labels) to > > the most common pattern (int instead of ssize_t/long) and adjust function > > return value accordingly. > > > > Signed-off-by: Peter Seiderer <ps.report@gmx.net> > > Hi Peter, > > These comments are is true in general of this patchset, but particularly so > in the case of this patch: > > * I think a more succinct subject would be nice. > * I think the patch description should provide some reason > _why_ the change is being made. Yep, will improve... > > Also, specifically relating to this patch, I wonder if it's scope ought to > be extended. For example, the two callers of num_arg(), get_imix_entries() and > pktgen_if_write() assign the return value of num_arg() to len, which is now > an int in both functions. But num_args() returns a long. Aim was to get rid of the int/long mixture in the code (which works flawless because no one writes to proc with more than a few bytes AND count is limited to INT_MAX - PAGE_SIZE in vfs_write (see [1], [2])... I believe the clean way is to use size_t i, max; ssize_t len; consequently through out the code and adjust the function signatures accordingly...., will re-spin... > > > --- > > Changes v3 -> v4 > > - new patch (factored out of patch 'net: pktgen: fix access outside of user > > given buffer in pktgen_if_write()') > > --- > > net/core/pktgen.c | 14 ++++++-------- > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > > index 4f201a2db2dc..279910367ad4 100644 > > --- a/net/core/pktgen.c > > +++ b/net/core/pktgen.c > > @@ -850,12 +850,11 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen) > > * where each entry consists of size and weight delimited by commas. > > * "size1,weight_1 size2,weight_2 ... size_n,weight_n" for example. > > */ > > -static ssize_t get_imix_entries(const char __user *buffer, > > - struct pktgen_dev *pkt_dev) > > +static int get_imix_entries(const char __user *buffer, > > + struct pktgen_dev *pkt_dev) > > { > > - int i = 0; > > - long len; > > char c; > > + int i = 0, len; > > Given it can be achieved with exactly the same lines changed, just in a > different order, please arrange the local variable declarations in reverse > xmas tree order - longest line to shortest. > > Likewise for the other hunk of this patch. And I believe there are also > other cases in this patchset where this comment applied. > > The following tool can be useful: > https://github.com/ecree-solarflare/xmastree O.k. will take a look at it... Thanks for review! Regards, Peter [1] https://elixir.bootlin.com/linux/v6.13.1/source/fs/read_write.c#L673 [2] https://elixir.bootlin.com/linux/v6.13.1/source/include/linux/fs.h#L2704
On Tue, Feb 11, 2025 at 10:29:59AM +0100, Peter Seiderer wrote: > Hello Simon, > > On Thu, 6 Feb 2025 13:25:38 +0000, Simon Horman <horms@kernel.org> wrote: > > > On Wed, Feb 05, 2025 at 02:11:45PM +0100, Peter Seiderer wrote: > > > Align some variable declarations (in get_imix_entries and get_labels) to > > > the most common pattern (int instead of ssize_t/long) and adjust function > > > return value accordingly. > > > > > > Signed-off-by: Peter Seiderer <ps.report@gmx.net> > > > > Hi Peter, > > > > These comments are is true in general of this patchset, but particularly so > > in the case of this patch: > > > > * I think a more succinct subject would be nice. > > * I think the patch description should provide some reason > > _why_ the change is being made. > > Yep, will improve... > > > > > Also, specifically relating to this patch, I wonder if it's scope ought to > > be extended. For example, the two callers of num_arg(), get_imix_entries() and > > pktgen_if_write() assign the return value of num_arg() to len, which is now > > an int in both functions. But num_args() returns a long. > > Aim was to get rid of the int/long mixture in the code (which works flawless > because no one writes to proc with more than a few bytes AND count is limited > to INT_MAX - PAGE_SIZE in vfs_write (see [1], [2])... > > I believe the clean way is to use > > size_t i, max; > ssize_t len; > > consequently through out the code and adjust the function signatures > accordingly...., will re-spin... Thanks Peter, I for one am all for things being consistent. ...
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 4f201a2db2dc..279910367ad4 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -850,12 +850,11 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen) * where each entry consists of size and weight delimited by commas. * "size1,weight_1 size2,weight_2 ... size_n,weight_n" for example. */ -static ssize_t get_imix_entries(const char __user *buffer, - struct pktgen_dev *pkt_dev) +static int get_imix_entries(const char __user *buffer, + struct pktgen_dev *pkt_dev) { - int i = 0; - long len; char c; + int i = 0, len; pkt_dev->n_imix_entries = 0; @@ -900,12 +899,11 @@ static ssize_t get_imix_entries(const char __user *buffer, return i; } -static ssize_t get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev) +static int get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev) { - unsigned int n = 0; char c; - ssize_t i = 0; - int len; + int i = 0, len; + unsigned int n = 0; pkt_dev->nr_labels = 0; do {
Align some variable declarations (in get_imix_entries and get_labels) to the most common pattern (int instead of ssize_t/long) and adjust function return value accordingly. Signed-off-by: Peter Seiderer <ps.report@gmx.net> --- Changes v3 -> v4 - new patch (factored out of patch 'net: pktgen: fix access outside of user given buffer in pktgen_if_write()') --- net/core/pktgen.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)