[v2,85/86] numa: make exit() usage consistent
diff mbox series

Message ID 1579100861-73692-86-git-send-email-imammedo@redhat.com
State New
Headers show
Series
  • refactor main RAM allocation to use hostmem backend
Related show

Commit Message

Igor Mammedov Jan. 15, 2020, 3:07 p.m. UTC
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: ehabkost@redhat.com
---
 hw/core/numa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Philippe Mathieu-Daudé Jan. 16, 2020, 3:40 p.m. UTC | #1
On 1/15/20 4:07 PM, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: ehabkost@redhat.com
> ---
>   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()) {
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Thomas Huth Jan. 16, 2020, 4:43 p.m. UTC | #2
On 15/01/2020 16.07, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: ehabkost@redhat.com
> ---
>  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.

 Thomas
Igor Mammedov Jan. 16, 2020, 5:10 p.m. UTC | #3
On Thu, 16 Jan 2020 17:43:30 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 15/01/2020 16.07, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > CC: ehabkost@redhat.com
> > ---
> >  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.

Ok, will do.

There are other patches that introduce new exit(EXIT_FAILURE),
is it fine to use that or should I stick to the style used in nearby code?

> 
>  Thomas
Thomas Huth Jan. 17, 2020, 7:24 a.m. UTC | #4
On 16/01/2020 18.10, Igor Mammedov wrote:
> On Thu, 16 Jan 2020 17:43:30 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 15/01/2020 16.07, Igor Mammedov wrote:
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> CC: ehabkost@redhat.com
>>> ---
>>>  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.
> 
> Ok, will do.
> 
> There are other patches that introduce new exit(EXIT_FAILURE),
> is it fine to use that or should I stick to the style used in nearby code?

Since we don't have a consensus yet, I guess it's ok to use it ... but
adapting to the surrounding code is also a good idea, of course.

 Thomas
Philippe Mathieu-Daudé Jan. 17, 2020, 8:06 a.m. UTC | #5
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 <imammedo@redhat.com>
>> ---
>> CC: ehabkost@redhat.com
>> ---
>>   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?

Thanks,

Phil.
Thomas Huth Jan. 17, 2020, 8:26 a.m. UTC | #6
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 <imammedo@redhat.com>
>>> ---
>>> CC: ehabkost@redhat.com
>>> ---
>>>   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.

 Thanks,
  Thomas
Thomas Huth Jan. 17, 2020, 8:30 a.m. UTC | #7
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 <imammedo@redhat.com>
>>>> ---
>>>> CC: ehabkost@redhat.com
>>>> ---
>>>>   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

Patch
diff mbox series

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()) {