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:
>>>> 
>>>>> 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 :)

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


>>>> * HACKING does not explicitly point out that exit(1) is the preferred exit 
>>>> code.
>> 
>>> Does it need saying?  We don't seem to have a weird exit() problem in
>>> new code.
>> 
>>> The lower HACKING's signal-to-noise ratio gets, the fewer people will
>>> actually read it attentively.
>> 
>> Fine by me.

> Thanks!

Cheers,
  Lluis