From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754020AbbKWJ2a (ORCPT ); Mon, 23 Nov 2015 04:28:30 -0500 Received: from mail-lf0-f46.google.com ([209.85.215.46]:36417 "EHLO mail-lf0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751251AbbKWJ22 convert rfc822-to-8bit (ORCPT ); Mon, 23 Nov 2015 04:28:28 -0500 From: Rasmus Villemoes To: Andy Shevchenko Cc: Andrew Morton , linux-kernel@vger.kernel.org Subject: Re: [PATCH v1 5/7] test_hexdump: check all bytes in real buffer Organization: D03 References: <1447259718-19647-1-git-send-email-andriy.shevchenko@linux.intel.com> <1447259718-19647-6-git-send-email-andriy.shevchenko@linux.intel.com> <87lh9us3h4.fsf@rasmusvillemoes.dk> <1448038537.31665.176.camel@linux.intel.com> X-Hashcash: 1:20:151123:andriy.shevchenko@linux.intel.com::By1JZLqcM4LxZXTR:00000000000000000000000000000n6b X-Hashcash: 1:20:151123:linux-kernel@vger.kernel.org::wSDvap+GcDjeHStr:0000000000000000000000000000000000naf X-Hashcash: 1:20:151123:akpm@linux-foundation.org::+zi5IUMjkrvtMqWn:00000000000000000000000000000000000020Da Date: Mon, 23 Nov 2015 10:28:25 +0100 In-Reply-To: <1448038537.31665.176.camel@linux.intel.com> (Andy Shevchenko's message of "Fri, 20 Nov 2015 18:55:37 +0200") Message-ID: <87r3jhyshy.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Nov 20 2015, Andy Shevchenko 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