Re: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19

[Peter Maydell]

On 19 September 2016 at 13:47, Daniel P. Berrange <address@hidden> wrote:
> On Mon, Sep 19, 2016 at 05:33:57AM -0700, address@hidden wrote:
>> Hi,
>>
>> Your series seems to have some coding style problems. See output below for
>> more information:
>>
>> Type: series
>> Subject: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19
>> Message-id: address@hidden
>>
>> === TEST SCRIPT BEGIN ===
>> #!/bin/bash
>>
>> BASE=base
>> n=1
>> total=$(git log --oneline $BASE.. | wc -l)
>> failed=0
>>
>> # Useful git options
>> git config --local diff.renamelimit 0
>> git config --local diff.renames True
>>
>> commits="$(git log --format=%H --reverse $BASE..)"
>> for c in $commits; do
>>     echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
>>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; 
>> then
>>         failed=1
>>         echo
>>     fi
>>     n=$((n+1))
>> done
>>
>> exit $failed
>> === TEST SCRIPT END ===
>>
>> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
>> Switched to a new branch 'test'
>> bdd8574 crypto: add trace points for TLS cert verification
>> a5218f2 crypto: support more hash algorithms for pbkdf
>> 4483f4e crypto: increase default pbkdf2 time for luks to 2 seconds
>> c75443a crypto: remove bogus /= 2 for pbkdf iterations
>> 61e0f1a crypto: use correct derived key size when timing pbkdf
>> 616fc7b crypto: clear out buffer after timing pbkdf algorithm
>> d4a7ad1 crypto: make PBKDF iterations configurable for LUKS format
>> f56a5e3 crypto: use uint64_t for pbkdf iteration count parameters
>>
>> === OUTPUT BEGIN ===
>> Checking PATCH 1/8: crypto: use uint64_t for pbkdf iteration count 
>> parameters...
>> Checking PATCH 2/8: crypto: make PBKDF iterations configurable for LUKS 
>> format...
>> Checking PATCH 3/8: crypto: clear out buffer after timing pbkdf algorithm...
>> Checking PATCH 4/8: crypto: use correct derived key size when timing pbkdf...
>> Checking PATCH 5/8: crypto: remove bogus /= 2 for pbkdf iterations...
>> Checking PATCH 6/8: crypto: increase default pbkdf2 time for luks to 2 
>> seconds...
>> Checking PATCH 7/8: crypto: support more hash algorithms for pbkdf...
>> ERROR: if this code is redundant consider removing it
>> #222: FILE: tests/test-crypto-pbkdf.c:332:
>> +#if 0
>>
>> total: 1 errors, 0 warnings, 194 lines checked
>
> Peter, FWIW, I know about this style check error and I'm intentionally
> ignoring it as I consider it a false positive. IMHO we should probably
> downgrade that style check to a WARNING, not an ERROR. The message itself
> is indicating that maintainers have discretion to ignore it and thus it
> shouldn't be an error.

Well, I don't in general look at patchew complaints on pull
requests (we should probably make it stop sending them) on
the basis that the maintainer should have already done that
before accepting the patches. But in general I think that
"#if 0" should be an error because there's not really any
good reason for it. For instance in this case there's no
explanation anywhere in the file of why these particular
test cases are disabled or in what circumstances they might
ever in future be enabled. If there's a case for the code being
possibly enabled at compile time locally or in the future then
#if SOMETHING (like the #ifdef DEBUG checks) with some comment
explaining the situation; if there isn't then the code doesn't
need to be there at all.

(This doesn't mean I'm rejecting this pull request.)

thanks
-- PMM