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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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.

Indeed, and that's why I prefer the santizing to be done by the regexp
package.  Perl does it with escapes (anybody surprised?).  Emacs
provides a function regexp-quote.  sed provides... nothing.

Let me decipher the Perl regexp for you:

    ^ anchors to the beginning of the line

    \Q$(SRC_PATH)\E matches $(SRC_PATH) exactly (\Q disables pattern
    metacharacters until \E)

    / matches the / your script appends to $(SRC_PATH), which it
    receives as argument of -o

Clear as mud?

Feel free to adopt something else, just make it work :)

>> 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.

There's one in tests/qemu-iotests/common.config's _readlink(), but it
appears unused.

We also have perl scripts checkpatch.pl and get_maintainer.pl.

>>> 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.

Exactly.