Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1272980 > unrolled thread
| Started by | Rasmus Villemoes <linux@rasmusvillemoes.dk> |
|---|---|
| First post | 2015-11-19 11:10 +0100 |
| Last post | 2015-11-26 16:30 +0100 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v1 3/7] test_hexdump: go through all possible lengths of buffer Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-19 11:10 +0100
Re: [PATCH v1 3/7] test_hexdump: go through all possible lengths of buffer Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2015-11-20 18:00 +0100
Re: [PATCH v1 3/7] test_hexdump: go through all possible lengths of buffer Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-23 10:00 +0100
Re: [PATCH v1 3/7] test_hexdump: go through all possible lengths of buffer Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-26 16:30 +0100
| From | Rasmus Villemoes <linux@rasmusvillemoes.dk> |
|---|---|
| Date | 2015-11-19 11:10 +0100 |
| Subject | Re: [PATCH v1 3/7] test_hexdump: go through all possible lengths of buffer |
| Message-ID | <qwuk9-7U6-11@gated-at.bofh.it> |
On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> When test for overflow do iterate the buffer length in a range
> 0 .. BUF_SIZE.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> lib/test_hexdump.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
> index ed7c6a7..15a6440 100644
> --- a/lib/test_hexdump.c
> +++ b/lib/test_hexdump.c
> @@ -126,17 +126,17 @@ static void __init test_hexdump_set(int rowsize, bool ascii)
> test_hexdump(len, rowsize, 1, ascii);
> }
>
> -static void __init test_hexdump_overflow(bool ascii)
> +static void __init test_hexdump_overflow(size_t buflen, bool ascii)
> {
> - char buf[56];
> + char buf[TEST_HEXDUMP_BUF_SIZE];
> const char *t = test_data_1_le[0];
> - size_t l = get_random_int() % sizeof(buf);
> + size_t l = buflen;
> bool a;
> int e, r;
>
> memset(buf, ' ', sizeof(buf));
>
> - r = hex_dump_to_buffer(data_b, 1, 16, 1, buf, l, ascii);
> + r = hex_dump_to_buffer(data_b, 1, 16, 1, buf, buflen, ascii);
>
> if (ascii)
> e = 50;
> @@ -144,7 +144,7 @@ static void __init test_hexdump_overflow(bool ascii)
> e = 2;
> buf[e + 2] = '\0';
>
> - if (!l) {
> + if (!buflen) {
> a = r == e && buf[0] == ' ';
> } else if (l < 3) {
> a = r == e && buf[0] == '\0';
Why keep the variable l when it is just a synonym for the new
parameter buflen? It is quite confusing that you change some but not
all occurrences of l to buflen. If you want to make the diff minimal
but still have a descriptive parameter name, just keep the 'size_t l =
buflen;' assignment and don't otherwise refer to buflen. But I think
it's better to eliminate 'l' and just change everything to
buflen. Don't mix the two approaches, though.
Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Andy Shevchenko <andriy.shevchenko@linux.intel.com> |
|---|---|
| Date | 2015-11-20 18:00 +0100 |
| Subject | Re: [PATCH v1 3/7] test_hexdump: go through all possible lengths of buffer |
| Message-ID | <qwXcu-1li-19@gated-at.bofh.it> |
| In reply to | #1272980 |
On Thu, 2015-11-19 at 11:07 +0100, Rasmus Villemoes wrote:
> On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.c
> om> wrote:
>
> > When test for overflow do iterate the buffer length in a range
> > 0 .. BUF_SIZE.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > lib/test_hexdump.c | 20 ++++++++++----------
> > 1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
> > index ed7c6a7..15a6440 100644
> > --- a/lib/test_hexdump.c
> > +++ b/lib/test_hexdump.c
> > @@ -126,17 +126,17 @@ static void __init test_hexdump_set(int
> > rowsize, bool ascii)
> > test_hexdump(len, rowsize, 1, ascii);
> > }
> >
> > -static void __init test_hexdump_overflow(bool ascii)
> > +static void __init test_hexdump_overflow(size_t buflen, bool
> > ascii)
> > {
> > - char buf[56];
> > + char buf[TEST_HEXDUMP_BUF_SIZE];
> > const char *t = test_data_1_le[0];
> > - size_t l = get_random_int() % sizeof(buf);
> > + size_t l = buflen;
> > bool a;
> > int e, r;
> >
> > memset(buf, ' ', sizeof(buf));
> >
> > - r = hex_dump_to_buffer(data_b, 1, 16, 1, buf, l, ascii);
> > + r = hex_dump_to_buffer(data_b, 1, 16, 1, buf, buflen,
> > ascii);
> >
> > if (ascii)
> > e = 50;
> > @@ -144,7 +144,7 @@ static void __init test_hexdump_overflow(bool
> > ascii)
> > e = 2;
> > buf[e + 2] = '\0';
> >
> > - if (!l) {
> > + if (!buflen) {
> > a = r == e && buf[0] == ' ';
> > } else if (l < 3) {
> > a = r == e && buf[0] == '\0';
>
>
> Why keep the variable l when it is just a synonym for the new
> parameter buflen? It is quite confusing that you change some but not
> all occurrences of l to buflen. If you want to make the diff minimal
> but still have a descriptive parameter name, just keep the 'size_t l
> =
> buflen;' assignment and don't otherwise refer to buflen. But I think
> it's better to eliminate 'l' and just change everything to
> buflen. Don't mix the two approaches, though.
Okay, I got it for the future, though the series is already in linux-
next, so do we really need to re-hack half of it because of that?
I suppose everything else you noticed I may send as one follow up
patch.
>
> Rasmus
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Rasmus Villemoes <linux@rasmusvillemoes.dk> |
|---|---|
| Date | 2015-11-23 10:00 +0100 |
| Message-ID | <qxV8B-7TP-5@gated-at.bofh.it> |
| In reply to | #1274266 |
On Fri, Nov 20 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Thu, 2015-11-19 at 11:07 +0100, Rasmus Villemoes wrote: >> >> >> Why keep the variable l when it is just a synonym for the new >> parameter buflen? It is quite confusing that you change some but not >> all occurrences of l to buflen. If you want to make the diff minimal >> but still have a descriptive parameter name, just keep the 'size_t l >> = >> buflen;' assignment and don't otherwise refer to buflen. But I think >> it's better to eliminate 'l' and just change everything to >> buflen. Don't mix the two approaches, though. > > Okay, I got it for the future, though the series is already in linux- > next, so do we really need to re-hack half of it because of that? Stuff in -next isn't set in stone. I'm pretty sure Andrew can replace one set of patches with another. Rasmus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2015-11-26 16:30 +0100 |
| Message-ID | <qz6EG-6XT-19@gated-at.bofh.it> |
| In reply to | #1275129 |
On Mon, Nov 23, 2015 at 10:59 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > On Fri, Nov 20 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> On Thu, 2015-11-19 at 11:07 +0100, Rasmus Villemoes wrote: >>> >>> >>> Why keep the variable l when it is just a synonym for the new >>> parameter buflen? It is quite confusing that you change some but not >>> all occurrences of l to buflen. If you want to make the diff minimal >>> but still have a descriptive parameter name, just keep the 'size_t l >>> = >>> buflen;' assignment and don't otherwise refer to buflen. But I think >>> it's better to eliminate 'l' and just change everything to >>> buflen. Don't mix the two approaches, though. >> >> Okay, I got it for the future, though the series is already in linux- >> next, so do we really need to re-hack half of it because of that? > > Stuff in -next isn't set in stone. I'm pretty sure Andrew can replace > one set of patches with another. Thanks for your review. I;m busy currently with some other stuff, you might have noticed, I will return back to the series maybe next week. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web