| Message ID | 20200207110259.12544-1-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vl: Abort if multiple machines are registered as default | expand |
Le 07/02/2020 à 12:02, Philippe Mathieu-Daudé a écrit : > It would be confusing to have multiple default machines. > Abort if this ever occurs. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > vl.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/vl.c b/vl.c > index 7dcb0879c4..da828188eb 100644 > --- a/vl.c > +++ b/vl.c > @@ -2354,6 +2354,8 @@ static MachineClass *machine_parse(const char *name, GSList *machines) > GSList *el; > > if (is_help_option(name)) { > + int default_count = 0; > + > printf("Supported machines are:\n"); > machines = g_slist_sort(machines, machine_class_cmp); > for (el = machines; el; el = el->next) { > @@ -2364,6 +2366,11 @@ static MachineClass *machine_parse(const char *name, GSList *machines) > printf("%-20s %s%s%s\n", mc->name, mc->desc, > mc->is_default ? " (default)" : "", > mc->deprecation_reason ? " (deprecated)" : ""); > + default_count += !!mc->is_default; > + } > + if (default_count > 1) { > + error_printf("Multiple default machines available\n"); > + abort(); > } > exit(0); > } > Does it really deserve an abort? Ideal solution would be to be able to check this at build or in the unit tests. Thanks, Laurent
Hi On Fri, Feb 7, 2020 at 12:03 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > It would be confusing to have multiple default machines. > Abort if this ever occurs. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > vl.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/vl.c b/vl.c > index 7dcb0879c4..da828188eb 100644 > --- a/vl.c > +++ b/vl.c > @@ -2354,6 +2354,8 @@ static MachineClass *machine_parse(const char *name, GSList *machines) > GSList *el; > > if (is_help_option(name)) { > + int default_count = 0; > + > printf("Supported machines are:\n"); > machines = g_slist_sort(machines, machine_class_cmp); > for (el = machines; el; el = el->next) { > @@ -2364,6 +2366,11 @@ static MachineClass *machine_parse(const char *name, GSList *machines) > printf("%-20s %s%s%s\n", mc->name, mc->desc, > mc->is_default ? " (default)" : "", > mc->deprecation_reason ? " (deprecated)" : ""); > + default_count += !!mc->is_default; > + } > + if (default_count > 1) { > + error_printf("Multiple default machines available\n"); > + abort(); looks ok It's a build-time issue? If the user can't do anything about it, you may simply have an assert(default_count <= 1) rather than a human-friendly string, I think.
On 2/7/20 12:08 PM, Laurent Vivier wrote: > Le 07/02/2020 à 12:02, Philippe Mathieu-Daudé a écrit : >> It would be confusing to have multiple default machines. >> Abort if this ever occurs. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> vl.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/vl.c b/vl.c >> index 7dcb0879c4..da828188eb 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -2354,6 +2354,8 @@ static MachineClass *machine_parse(const char *name, GSList *machines) >> GSList *el; >> >> if (is_help_option(name)) { >> + int default_count = 0; >> + >> printf("Supported machines are:\n"); >> machines = g_slist_sort(machines, machine_class_cmp); >> for (el = machines; el; el = el->next) { >> @@ -2364,6 +2366,11 @@ static MachineClass *machine_parse(const char *name, GSList *machines) >> printf("%-20s %s%s%s\n", mc->name, mc->desc, >> mc->is_default ? " (default)" : "", >> mc->deprecation_reason ? " (deprecated)" : ""); >> + default_count += !!mc->is_default; >> + } >> + if (default_count > 1) { >> + error_printf("Multiple default machines available\n"); >> + abort(); >> } >> exit(0); >> } >> > > Does it really deserve an abort? > Ideal solution would be to be able to check this at build or in the unit > tests. This is for developers, not for users. I'll use Marc-André suggestion and use an assertion.
Le 07/02/2020 à 12:25, Philippe Mathieu-Daudé a écrit : > On 2/7/20 12:08 PM, Laurent Vivier wrote: >> Le 07/02/2020 à 12:02, Philippe Mathieu-Daudé a écrit : >>> It would be confusing to have multiple default machines. >>> Abort if this ever occurs. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> vl.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/vl.c b/vl.c >>> index 7dcb0879c4..da828188eb 100644 >>> --- a/vl.c >>> +++ b/vl.c >>> @@ -2354,6 +2354,8 @@ static MachineClass *machine_parse(const char >>> *name, GSList *machines) >>> GSList *el; >>> if (is_help_option(name)) { >>> + int default_count = 0; >>> + >>> printf("Supported machines are:\n"); >>> machines = g_slist_sort(machines, machine_class_cmp); >>> for (el = machines; el; el = el->next) { >>> @@ -2364,6 +2366,11 @@ static MachineClass *machine_parse(const char >>> *name, GSList *machines) >>> printf("%-20s %s%s%s\n", mc->name, mc->desc, >>> mc->is_default ? " (default)" : "", >>> mc->deprecation_reason ? " (deprecated)" : ""); >>> + default_count += !!mc->is_default; >>> + } >>> + if (default_count > 1) { >>> + error_printf("Multiple default machines available\n"); >>> + abort(); >>> } >>> exit(0); >>> } >>> >> >> Does it really deserve an abort? >> Ideal solution would be to be able to check this at build or in the unit >> tests. > > This is for developers, not for users. I'll use Marc-André suggestion > and use an assertion. I agree, but it's why it would be better if it is detected before it comes to the user. Perhaps you can add a test to run "qemu -h" to trigger the abort() early in the develompment process (if it doesn't already exist). Thanks, Laurent Thanks, LAurent
Le 07/02/2020 à 12:28, Laurent Vivier a écrit : > Le 07/02/2020 à 12:25, Philippe Mathieu-Daudé a écrit : >> On 2/7/20 12:08 PM, Laurent Vivier wrote: >>> Le 07/02/2020 à 12:02, Philippe Mathieu-Daudé a écrit : >>>> It would be confusing to have multiple default machines. >>>> Abort if this ever occurs. >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> --- >>>> vl.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/vl.c b/vl.c >>>> index 7dcb0879c4..da828188eb 100644 >>>> --- a/vl.c >>>> +++ b/vl.c >>>> @@ -2354,6 +2354,8 @@ static MachineClass *machine_parse(const char >>>> *name, GSList *machines) >>>> GSList *el; >>>> if (is_help_option(name)) { >>>> + int default_count = 0; >>>> + >>>> printf("Supported machines are:\n"); >>>> machines = g_slist_sort(machines, machine_class_cmp); >>>> for (el = machines; el; el = el->next) { >>>> @@ -2364,6 +2366,11 @@ static MachineClass *machine_parse(const char >>>> *name, GSList *machines) >>>> printf("%-20s %s%s%s\n", mc->name, mc->desc, >>>> mc->is_default ? " (default)" : "", >>>> mc->deprecation_reason ? " (deprecated)" : ""); >>>> + default_count += !!mc->is_default; >>>> + } >>>> + if (default_count > 1) { >>>> + error_printf("Multiple default machines available\n"); >>>> + abort(); >>>> } >>>> exit(0); >>>> } >>>> >>> >>> Does it really deserve an abort? >>> Ideal solution would be to be able to check this at build or in the unit >>> tests. >> >> This is for developers, not for users. I'll use Marc-André suggestion >> and use an assertion. > > I agree, but it's why it would be better if it is detected before it > comes to the user. > > Perhaps you can add a test to run "qemu -h" to trigger the abort() early > in the develompment process (if it doesn't already exist). I mean "-M help". Thanks, Laurent
diff --git a/vl.c b/vl.c index 7dcb0879c4..da828188eb 100644 --- a/vl.c +++ b/vl.c @@ -2354,6 +2354,8 @@ static MachineClass *machine_parse(const char *name, GSList *machines) GSList *el; if (is_help_option(name)) { + int default_count = 0; + printf("Supported machines are:\n"); machines = g_slist_sort(machines, machine_class_cmp); for (el = machines; el; el = el->next) { @@ -2364,6 +2366,11 @@ static MachineClass *machine_parse(const char *name, GSList *machines) printf("%-20s %s%s%s\n", mc->name, mc->desc, mc->is_default ? " (default)" : "", mc->deprecation_reason ? " (deprecated)" : ""); + default_count += !!mc->is_default; + } + if (default_count > 1) { + error_printf("Multiple default machines available\n"); + abort(); } exit(0); }
It would be confusing to have multiple default machines. Abort if this ever occurs. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- vl.c | 7 +++++++ 1 file changed, 7 insertions(+)