Re: [PATCH v1 5/7] test_hexdump: check all bytes in real buffer

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

On Fri, Nov 20 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, 2015-11-19 at 11:11 +0100, Rasmus Villemoes wrote:
>> 
>> First of all, ' ' is one of the characters which hexdump is certainly
>> supposed to spit out, so I think it's better to use some other
>> character for prefilling. Otherwise we wouldn't be able to detect a
>> stray write of a space which wasn't properly guarded by a size
>> check. I'd suggest '\xff' or any other non-ascii
>
> Okay, I may change the ' ' to something, but somehow printable. See
> also below.
>
>> > -			a = r == e && buf[l - 1] == '\0' && buf[l
>> > - 2] == ' ';
>> > -		else
>> > -			a = r == e && buf[50] == '\0' && buf[49]
>> > == '.';
>> > +		memset(test, ' ', sizeof(test));
>> > +		test[sizeof(buf) - 1] = '\0';
>> > +
>> > +		a = r == e && !memchr_inv(buf, ' ', sizeof(buf));
>> 
>> test and buf happen to have the same size, but
>> "test[sizeof(buf) - 1] = '\0'" is rather odd. But you don't even seem
>> to use test in this branch?
>
> Here I feel the test buffer just to print below if any error happens
> when buflen == 0.
>
> That's why I would like to have a somehow printable character.

Oh, but that's wrong! That is, printing the filled test buffer does not
actually show what was "expected", whether you were filling with spaces,
$ or \xff. I really can't think of a situation where you'd want to print
the supposedly unused part of the buffer, so the character used for
filling shouldn't matter.

In any case, the most important thing is to stay away from [ a-f0-9]
which hexdump is certainly expected to produce - the reason I suggested
a non-ascii character was because hexdump might produce any printable
ascii character in the ascii part (but I suppose one could use e.g. '#'
which isn't among the characters we feed it).

>> >  	} else {
>> > -		a = r == e && buf[e] == '\0';
>> > +		int f = min_t(int, e + 1, buflen);
>> > +
>> > +		test_hexdump_prepare_test(len, rs, gs, test,
>> > sizeof(test), ascii);
>> > +		test[f - 1] = '\0';
>> > +
>> > +		a = r == e && !memchr_inv(buf + f, ' ',
>> > sizeof(buf) - f) && !strcmp(buf, test);
>> >  	}
>> 
>> There's also a bit of duplication in the !buflen and buflen
>> branches. Why not pull the computation of f (the number of expected
>> bytes written) outside and do
>
> See above. buflen == 0 is a special case where buffer shouldn't be
> touched at all.

No, buflen==0 is not that special. We expect hexdump to touch exactly
the first buflen bytes (or only e+1, whichever is smaller), and the
remaining bytes should be untouched. A memcmp handles the first part, a
memchr_inv the second.

Rasmus