Re: [Qemu-trivial] [RFC PATCH] vl: only display available accelerators

[Eduardo Habkost]

On Wed, Nov 08, 2017 at 01:20:49PM -0300, Philippe Mathieu-Daudé wrote:
> On 11/02/2017 09:35 PM, Eduardo Habkost wrote:
> > On Mon, Oct 30, 2017 at 03:14:59PM -0300, Philippe Mathieu-Daudé wrote:
> >> examples configuring with '--enable-kvm --disable-tcg'
> >>
> >> - before
> >>
> >>   $ qemu-system-x86_64 -accel help
> >>   Possible accelerators: kvm, xen, hax, tcg
> >>
> >>   $ qemu-system-x86_64 -accel tcg
> >>   qemu-system-x86_64: -machine accel=tcg: No accelerator found
> >>
> >>   # qemu-system-x86_64 -accel hax
> >>   qemu-system-x86_64: -machine accel=hax: No accelerator found
> >>
> >> - after
> >>
> >>   $ qemu-system-x86_64 -accel help
> >>   Possible accelerators:
> >>     xen
> >>     kvm
> >>
> >> Suggested-by: Eduardo Habkost <address@hidden>
> >> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> >> ---
> >> since RFC:
> >>   - use much cleaner object_class_get_list(TYPE_ACCEL, false)
> >>
> >>  vl.c | 39 ++++++++++++++++++++++++++++++++++-----
> >>  1 file changed, 34 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/vl.c b/vl.c
> >> index ec299099ff..0f13641715 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -2764,6 +2764,39 @@ static gint machine_class_cmp(gconstpointer a, 
> >> gconstpointer b)
> >>      exit(!name || !is_help_option(name));
> >>  }
> >>  
> >> +static void accel_list_entry(gpointer data, gpointer user_data)
> >> +{
> >> +    ObjectClass *oc = data;
> >> +    const char *typename = object_class_get_name(oc);
> >> +    int len;
> >> +
> >> +    if (!qtest_driver() && !g_strcmp0(typename, 
> >> ACCEL_CLASS_NAME("qtest"))) {
> >> +        return; /* used by test cases */
> >> +    }
> > 
> > I would prefer to not hardcode the accel name here.  If you want
> > qtest to have special behavior, you can add a new field to
> > AccelClass to implement that.
> > 
> > However, I don't think I agree we should hide qtest from the
> > user: it is accepted on the command-line, so why should we lie?
> 
> Ok.
> 
> > Why not add a description field and indicate that qtest is useful
> > only for testing?
> 
> This is cleaner indeed.
> 
> >> +
> >> +    len = strlen(typename) - strlen("-" TYPE_ACCEL);
> > 
> > You can use AccelClass::name here.
> 
> Hmm yes but currently this is not what the user should use, since the
> accel_find() matches the ACCEL_CLASS_NAME:
> 
> $ ppc64-softmmu/qemu-system-ppc64 -accel QTest
> qemu-system-ppc64: -machine accel=QTest: No accelerator found

Oops, you're correct.

In this case, I suggest using ACCEL_CLASS_SUFFIX instead of
hardcoding ("-" TYPE_ACCEL).

Maybe a accel_class_get_accel_name() helper similar to
x86_cpu_class_get_model_name() would be even better.  It would
require an extra memory allocation, but the code would be
cleaner.  We might add a QMP command returning the accel list in
the future, so a common helper would be useful.


> 
> I'll respin but the change will be bigger so I is probably too late for
> 2.11.
> 
> > 
> >> +    if (len > 0) {
> >> +        error_printf("  %.*s\n", len, typename);
> >> +    }
> >> +}
> >> +
> >> +static void accel_parse(const char *name, QemuOpts *accel_opts)
> >> +{
> >> +    const char *optarg = qemu_opt_get(accel_opts, "accel");
> >> +    GSList *list;
> >> +
> >> +    if (!is_help_option(optarg)) {
> >> +        return;
> >> +    }
> >> +
> >> +    list = object_class_get_list(TYPE_ACCEL, false);
> >> +    error_printf("Possible accelerators:\n");
> >> +    g_slist_foreach(list, accel_list_entry, NULL);
> >> +    g_slist_free(list);
> >> +
> >> +    exit(0);
> >> +}
> >> +
> >>  void qemu_add_exit_notifier(Notifier *notify)
> >>  {
> >>      notifier_list_add(&exit_notifiers, notify);
> >> @@ -3881,11 +3914,7 @@ int main(int argc, char **argv, char **envp)
> >>              case QEMU_OPTION_accel:
> >>                  accel_opts = 
> >> qemu_opts_parse_noisily(qemu_find_opts("accel"),
> >>                                                       optarg, true);
> >> -                optarg = qemu_opt_get(accel_opts, "accel");
> >> -                if (!optarg || is_help_option(optarg)) {
> >> -                    error_printf("Possible accelerators: kvm, xen, hax, 
> >> tcg\n");
> >> -                    exit(0);
> >> -                }
> >> +                accel_parse(optarg, accel_opts);
> >>                  opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
> >>                                          false, &error_abort);
> >>                  qemu_opt_set(opts, "accel", optarg, &error_abort);
> >> -- 
> >> 2.15.0.rc2
> >>
> > 

-- 
Eduardo