Re: [Qemu-devel] [PATCH 0/2] error: Documentation updates

[Lluís Vilanova]

Markus Armbruster writes:

> Lluís Vilanova <address@hidden> writes:
>> Markus Armbruster writes:
>> 
>>> Lluís Vilanova <address@hidden> writes:
>>>> Markus Armbruster writes:
>>>> 
>>>>> Lluís Vilanova <address@hidden> writes:
>>>>>> Markus Armbruster writes:
>>>>>> 
>>>>>>> This overlaps with parts of Lluís's "[RFC][PATCH v6 0/5] utils:
>>>>>>> Improve and document error reporting".  Lluís, feel free to integrate
>>>>>>> my patches into a respin of your series.  You can also respin on top
>>>>>>> of my series.
>>>>>> 
>>>>>> Reviewed-by: Lluís Vilanova <address@hidden>
>>>>>> 
>>>>>> I'm happy with this series as a replacement for mine. Two nitpicks:
>>>>>> 
>>>>>> * The error.h comments point to assert() instead of abort() as a 
>>>>>> replacement for
>>>>>> error_setg(&error_abort) (while your HACKING says otherwise).
>>>> 
>>>>> Where?
>>>> 
>>>> Documentation on error_setg() (last phrase) vs HACKING (also last phrase).
>> 
>>> Got it.  error_setg()'s comment:
>> 
>>> Likewise, don't error_setg(&error_abort, ...), use assert().
>> 
>>> This is advice on what to do.
>> 
>>> HACKING:
>> 
>>> Note that &error_fatal is just another way to exit(1), and
>>> &error_abort is just another way to abort().
>> 
>>> This tries to extend the admonition on exit() and abort() to
>>> &error_fatal and &error_abort.  I'm open for better ways to word it.
>>> However, I feel this section of HACKING should not go into detail on
>>> what to do, but leave that to error.h.
>> 
>> Right. I just wanted to say the error.h comment should be:
>> 
>> Likewise, don't error_setg(&error_abort, ...), use abort().
>> 
>> To be consistent with HACKING (which implies the equivalent for error_abort 
>> is
>> abort()). But that's just me thinking that assert will be omitted when 
>> NDEBUG is
>> defined :)

> In my opinion, the error.h comment should point to assert(), not
> abort(), because &error_abort and assert() both print information on the
> error location, while abort() doesn't.  I *want* people to use assert().

> But I'd rather not explain all that in HACKING, to avoid getting bogged
> down in detail there.  Feel free to suggest a wording that improves
> consistency without getting too far into the weeds.

> HACKING doesn't say &error_abort is *equivalent* to abort(), it says
> "&error_abort is just another way to abort()".  That's true for
> assert(), too.  Except assert() comes with a compile time switch we
> don't use.

Well, now that I think of it, assert() uses abort() underneath, so they're
equivalent on that sense (not that I thought of it initially).

HACKING:

    Note that &error_fatal is just another way to exit(1), and &error_abort is
    just another way to terminate QEMU like abort() or assert().

Also, are we all aware that messages from abort() won't be redirected to the
monitor? (error_setg, error_report and so redirect appropriately). Just checking
if it makes sense to redirect them.

BTW, either way, I'm fine if you push this into upstream.


[...]
>> As a side note (just an observation), it seems that NDEBUG is only 
>> selectively
>> (and manually) defined in some files like "tcg/tcg.c", so it's not 
>> consistently
>> affecting files (e.g., it's used as a shorter substitute of "#ifdef
>> CONFIG_DEBUG_TCG", but it's not acting like a "real" NDEBUG; aka assert will
>> still work *iff* it's included before defining NDEBUG, which is not clear at
>> all).

> I can see two instances (tci.c tcg/tcg.c), and both look like this:

>     #if !defined(CONFIG_DEBUG_TCG) && !defined(NDEBUG)
>     # define NDEBUG
>     #endif

> They make switching off CONFIG_DEBUG_TCG switch off assertions as well.
> Feels odd to me, but I'm not familiar with these two files.

That's the case now, since assert.h is never directly (or indirectly) included
before the define. But it looks fragile to me; it's very easy to add an include
before that define.


Cheers,
  Lluis