| Message ID | 1579100861-73692-86-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | refactor main RAM allocation to use hostmem backend | expand |
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>
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
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
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
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.
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
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
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()) {
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- CC: ehabkost@redhat.com --- hw/core/numa.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)