Re: [Qemu-devel] [PATCH v2] checkpatch: add a little script to run checkpatch against a git refspec

[Anthony Liguori]

Eric Blake <address@hidden> writes:

> On 01/21/2013 12:20 PM, Anthony Liguori wrote:
>> This makes it easier to use checkpatch with a git hook or via patches.
>> 
>> Signed-off-by: Anthony Liguori <address@hidden>
>> ---
>> v1 -> v2
>>  - Add the subject to the output to indicate which patch failed
>>  - Only display output for patches that fail the check
>> ---
>>  scripts/check-patches.sh | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>  create mode 100755 scripts/check-patches.sh
>> 
>
>> +TMPFILE=/tmp/check-patches.out.$$
>
> This name is highly predictable, and thus rather insecure.  Is it worth
> using common idioms for safer temporary files?

I don't really consider this to be a problem but since I have to respin
anyway, I'll use mktemp -t.


>> +ret=0
>> +git log --format="%H %s" "$@" | while read LINE; do
>> +    commit="`echo $LINE | cut -f1 -d' '`"
>> +    subject="`echo $LINE | cut -f2- -d' '`"
>> +    echo "Subject: $subject"
>
> This won't work if $subject contains backslash.  You must use printf(1)
> to be portable here.

What won't work, echo or read?  -r should fix the read bit but echo
doesn't interpret newlines by default.... or is that a GNU-ism?

>
>> +    git show --format=email $commit | scripts/checkpatch.pl - > $TMPFILE
>> +    
>> +    rc=$?
>> +    if test $rc -ne 0; then
>> +    ret=rc
>> +    cat $TMPFILE
>> +    echo
>> +    fi
>> +done
>> +
>> +rm -f $TMPFILE
>> 
>
> Where do you use $ret?

That's a bug.

> Also, you are executing the assignment to ret
> inside a while loop that was part of a pipeline, but POSIX says a
> compliant shell might execute that loop in a subshell (and bash does
> just that), so that the parent shell cannot not see the change in the
> value of $ret.  If you really must propagate errors outside of the while
> loop, then instead of doing:
>
> command | while read; done
> use results from loop
>
> you have to instead use an alternative such as:
>
> command | { while read; done
>   use results from loop
> }

Indeed.  I didn't see this because of the above bug.

Regards,

Anthony Liguori

>
> or a named fifo (but that gets back to my question of how do you intend
> to generate a secure name for your fifo).
>
> http://mywiki.wooledge.org/BashFAQ/024
>
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org