Re: [PATCH v2 85/86] numa: make exit() usage consistent

[Thomas Huth]

On 17/01/2020 09.26, Thomas Huth wrote:
> On 17/01/2020 09.06, Philippe Mathieu-Daudé wrote:
>> Hi Thomas,
>>
>> On 1/16/20 5:43 PM, Thomas Huth wrote:
>>> On 15/01/2020 16.07, Igor Mammedov wrote:
>>>> Signed-off-by: Igor Mammedov <address@hidden>
>>>> ---
>>>> CC: address@hidden
>>>> ---
>>>>   hw/core/numa.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/core/numa.c b/hw/core/numa.c
>>>> index 3177066..47d5ea1 100644
>>>> --- a/hw/core/numa.c
>>>> +++ b/hw/core/numa.c
>>>> @@ -718,7 +718,7 @@ void numa_complete_configuration(MachineState *ms)
>>>>           /* Report large node IDs first, to make mistakes easier to
>>>> spot */
>>>>           if (!numa_info[i].present) {
>>>>               error_report("numa: Node ID missing: %d", i);
>>>> -            exit(1);
>>>> +            exit(EXIT_FAILURE);
>>>>           }
>>>>       }
>>>>   @@ -759,7 +759,7 @@ void numa_complete_configuration(MachineState *ms)
>>>>               error_report("total memory for NUMA nodes (0x%" PRIx64 ")"
>>>>                            " should equal RAM size (0x" RAM_ADDR_FMT
>>>> ")",
>>>>                            numa_total, ram_size);
>>>> -            exit(1);
>>>> +            exit(EXIT_FAILURE);
>>>>           }
>>>>             if (!numa_uses_legacy_mem()) {
>>>
>>> Please don't. We've had exit(1) vs. exit(EXIT_FAILURE) discussions in
>>> the past already, and IIRC there was no clear conclusion which one we
>>> want to use. There are examples of changes to the numeric value in our
>>> git history (see d54e4d7659ebecd0e1fa7ffc3e954197e09f8a1f for example),
>>> and example of the other way round (see 4d1275c24d5d64d22ec4a30ce1b6a0
>>> for example).
>>>
>>> Your patch series here is already big enough, so I suggest to drop this
>>> patch from the series. If you want to change this, please suggest an
>>> update to CODING_STYLE.rst first so that we agree upon one style for
>>> exit() ... otherwise somebody else might change this back into numeric
>>> values in a couple of months just because they have a different taste.
>>
>> TBH I find your suggestion a bit harsh. If you noticed this, it means
>> you care about finding a consensus about which style the project should
>> use, but then you ask Igor to update to CODING_STYLE to restart the
>> discussion until we get an agreement, so he can apply his patch.
>>
>> If this patch were single, this could be understandable. Now considering
>> the series size, as you suggested, as the patch author I'd obviously
>> drop my patch and stay away of modifying a 'exit()' line forever.
>>
>> Maybe it is a good opportunity to restart the discussion and settle on a
>> position, update CODING_STYLE.rst, do a global cleanup, update
>> checkpatch to keep the code clean.
>> As I don't remember such discussions, they might predate my involvement
>> with the project. Do you mind starting a thread with pointers to the
>> previous discussions?
> 
> Honestly, I don't care much whether we use exit(EXIT_FAILURE) or
> exit(1). But I care about having less code churn, so that "git blame"
> stays somewhat usable in the course of time, i.e. I really like to avoid
> that we include such ping-pong patches where every author changes such
> lines to their current taste.
> 
> Thus if someone really cares to change such matter-of-taste code lines,
> I think it's fair to ask them to update CODING_STYLE first. Otherwise,
> yes, please just leave the exit() lines as they are to avoid unnecessary
> code churn.

By the way:

$ grep -r 'exit(1)' * | wc -l
795
$ grep -r 'exit(EXIT_FAILURE)' * | wc -l
205

... that indicates that we should maybe rather go with exit(1) instead
of exit(EXIT_FAILURE).

 Thomas