Re: [Qemu-devel] [PATCH v9 3/4] qapi: Use an explicit input file

[Eric Blake]

On 04/29/2014 02:20 PM, Lluís Vilanova wrote:
> Markus Armbruster writes:
> 
>> Lluís Vilanova <address@hidden> writes:
>>> Use an explicit input file on the command-line instead of reading from 
>>> standard input
> 
>> Please limit commit message line length to 70 characters.
> 
>> Worth mentioning that this commit improves error messages!
> 
>>     <stdin>:123: Borked
> 
>> becomes
> 
>>     qapi-schema.json:123: Borked
> 
>> which enables Emacs to jump to the error.

>>> @diff -q $(SRC_PATH)/$*.out $*.test.out
>>> -   @diff -q $(SRC_PATH)/$*.err $*.test.err
>>> +   @# Sanitize error messages (make them independent of build directory)
>>> +   @sed 's|$(SRC_PATH)/||g' $*.test.err | diff -q $(SRC_PATH)/$*.err -
>>> @diff -q $(SRC_PATH)/$*.exit $*.test.exit
>>>
> 
>> Breaks when $(SRC_PATH) interpreted as regexp matches anything but a
>> prefix of the source file part.  In particular, it breaks when SRC_PATH
>> is ".." (which is common) and the error message contains "/".
> 
>> Here's a safer version:
> 
>>     @perl -p -e 's|^\Q$(SRC_PATH)\E/||' $*.test.err | diff -q 
>> $(SRC_PATH)/$*.err -

I guess there's other ways to avoid the problems without using perl
(such as sanitizing all metacharacters before passing a munged
$(SRC_PATH) to sed), but it gets hairy.

> 
> Not a perl fan, so I'll assume your line as correct. Also, can perl be assumed
> as present?

Makefile already uses perl to generate documentation from .pod files;
although this appears to be the first use of perl in the testsuite.


> 
>> This one reports missing input file name argument as "IndexError: list
>> index out of range".  Again, fits right in.
> 
>> [...]
> 
> AFAIR, Eric said that it'd be better to leave it as simple as possible, since 
> no
> one else uses this script.
> 
> Same applies to the argument parsing of the other scripts (since no one is
> sufficiently annoyed to rewrite it with argparse or similar).

Careful - I think that keeping _this patch_ simple is okay, but I would
also _welcome_ further patches in this (or other) series that further
improves the error message quality and consistency when using the
various .py generators from the command line.  I think the point we are
making here is that your code has very ugly inconsistent results when
detecting errors across the various scripts, but that 1) it's no uglier
than pre-patch, and 2) in the normal case, 'make' and 'make check' will
not trigger these ugly error paths.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature