[PATCH] Disallow colons in the parameter of "-accel"

[PATCH] Disallow colons in the parameter of "-accel"

[Thomas Huth]

Everybody who used something like "-machine accel=kvm:tcg" in the past
might be tempted to specify a similar list with the -accel parameter,
too, for example "-accel kvm:tcg". However, this is not how this
options is thought to be used, since each "-accel" should only take care
of one specific accelerator.

In the long run, we really should rework the "-accel" code completely,
so that it does not set "-machine accel=..." anymore internally, but
is completely independent from "-machine". For the short run, let's
make sure that users cannot use "-accel xyz:tcg", so that we avoid
that we have to deal with such cases in the wild later.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 vl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/vl.c b/vl.c
index 630f5c5e9c..68f47a9c25 100644
--- a/vl.c
+++ b/vl.c
@@ -3554,6 +3554,11 @@ int main(int argc, char **argv, char **envp)
                     g_slist_free(accel_list);
                     exit(0);
                 }
+                if (optarg && strchr(optarg, ':')) {
+                    error_report("Don't use ':' with -accel, "
+                                 "use -M accel=... in this case instead");
+                    exit(1);
+                }
                 opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
                                         false, &error_abort);
                 qemu_opt_set(opts, "accel", optarg, &error_abort);
-- 
2.18.1

Re: [PATCH] Disallow colons in the parameter of "-accel"

[Peter Maydell]

On Mon, 23 Sep 2019 at 13:21, Thomas Huth <thuth@redhat.com> wrote:
>
> Everybody who used something like "-machine accel=kvm:tcg" in the past
> might be tempted to specify a similar list with the -accel parameter,
> too, for example "-accel kvm:tcg". However, this is not how this
> options is thought to be used, since each "-accel" should only take care
> of one specific accelerator.
>
> In the long run, we really should rework the "-accel" code completely,
> so that it does not set "-machine accel=..." anymore internally, but
> is completely independent from "-machine". For the short run, let's
> make sure that users cannot use "-accel xyz:tcg", so that we avoid
> that we have to deal with such cases in the wild later.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  vl.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/vl.c b/vl.c
> index 630f5c5e9c..68f47a9c25 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3554,6 +3554,11 @@ int main(int argc, char **argv, char **envp)
>                      g_slist_free(accel_list);
>                      exit(0);
>                  }
> +                if (optarg && strchr(optarg, ':')) {
> +                    error_report("Don't use ':' with -accel, "
> +                                 "use -M accel=... in this case instead");
> +                    exit(1);
> +                }

This seems pretty ugly. If -accel is the way we're recommending
users configure the accelerator then it should support syntax
for specifying everything we could do with the old -machine...
option. If it can't do that yet, we shouldn't switch over to it
until it can.

thanks
-- PMM

Re: [PATCH] Disallow colons in the parameter of "-accel"

[Thomas Huth]

On 23/09/2019 14.23, Peter Maydell wrote:
> On Mon, 23 Sep 2019 at 13:21, Thomas Huth <thuth@redhat.com> wrote:
>>
>> Everybody who used something like "-machine accel=kvm:tcg" in the past
>> might be tempted to specify a similar list with the -accel parameter,
>> too, for example "-accel kvm:tcg". However, this is not how this
>> options is thought to be used, since each "-accel" should only take care
>> of one specific accelerator.
>>
>> In the long run, we really should rework the "-accel" code completely,
>> so that it does not set "-machine accel=..." anymore internally, but
>> is completely independent from "-machine". For the short run, let's
>> make sure that users cannot use "-accel xyz:tcg", so that we avoid
>> that we have to deal with such cases in the wild later.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  vl.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/vl.c b/vl.c
>> index 630f5c5e9c..68f47a9c25 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3554,6 +3554,11 @@ int main(int argc, char **argv, char **envp)
>>                      g_slist_free(accel_list);
>>                      exit(0);
>>                  }
>> +                if (optarg && strchr(optarg, ':')) {
>> +                    error_report("Don't use ':' with -accel, "
>> +                                 "use -M accel=... in this case instead");
>> +                    exit(1);
>> +                }
> 
> This seems pretty ugly.

Yes. The whole "-accel" option is currently ugly. My patch is just a
temporary work-around to prevent that we later have to deal with the
fact that users started to use this colon here in the wild and we would
then have to fight to get rid of it again.

> If -accel is the way we're recommending
> users configure the accelerator then it should support syntax
> for specifying everything we could do with the old -machine...
> option.

No, we certainly don't want to have the colon in here. The idea is
rather that you could specify multiply "-accel" options one day, e.g.:

 -accel tcg,tb-size=2048 -accel kvm,kernel_irqchip=on

... and then the accelators are used with the right parameters in the
order of availability.

Per-accelerator parameters just don't work here if you allow the colon.

(IMHO we should not have let the -accel code enter the repository in
this shape, but rather insist on a proper implementation right from the
start - but now that it's there, we have to deal with it and should make
sure that it does not get worse)

 Thomas

Re: [PATCH] Disallow colons in the parameter of "-accel"

[Peter Maydell]

On Mon, 23 Sep 2019 at 13:42, Thomas Huth <thuth@redhat.com> wrote:
>
> On 23/09/2019 14.23, Peter Maydell wrote:
> > This seems pretty ugly.
>
> Yes. The whole "-accel" option is currently ugly. My patch is just a
> temporary work-around to prevent that we later have to deal with the
> fact that users started to use this colon here in the wild and we would
> then have to fight to get rid of it again.
>
> > If -accel is the way we're recommending
> > users configure the accelerator then it should support syntax
> > for specifying everything we could do with the old -machine...
> > option.
>
> No, we certainly don't want to have the colon in here. The idea is
> rather that you could specify multiply "-accel" options one day, e.g.:
>
>  -accel tcg,tb-size=2048 -accel kvm,kernel_irqchip=on
>
> ... and then the accelators are used with the right parameters in the
> order of availability.
>
> Per-accelerator parameters just don't work here if you allow the colon.
>
> (IMHO we should not have let the -accel code enter the repository in
> this shape, but rather insist on a proper implementation right from the
> start - but now that it's there, we have to deal with it and should make
> sure that it does not get worse)

Ah, I hadn't realised that -accel was a (relatively) long-standing
option; I'd just noticed some patches going past recently suggesting
we were starting to recommend it over -machine accel=.
Thanks for trying to clean it up a bit.

-- PMM

Re: [PATCH] Disallow colons in the parameter of "-accel"

[Paolo Bonzini]

On 23/09/19 14:17, Thomas Huth wrote:
> Everybody who used something like "-machine accel=kvm:tcg" in the past
> might be tempted to specify a similar list with the -accel parameter,
> too, for example "-accel kvm:tcg". However, this is not how this
> options is thought to be used, since each "-accel" should only take care
> of one specific accelerator.
> 
> In the long run, we really should rework the "-accel" code completely,
> so that it does not set "-machine accel=..." anymore internally, but
> is completely independent from "-machine". For the short run, let's
> make sure that users cannot use "-accel xyz:tcg", so that we avoid
> that we have to deal with such cases in the wild later.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  vl.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/vl.c b/vl.c
> index 630f5c5e9c..68f47a9c25 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3554,6 +3554,11 @@ int main(int argc, char **argv, char **envp)
>                      g_slist_free(accel_list);
>                      exit(0);
>                  }
> +                if (optarg && strchr(optarg, ':')) {
> +                    error_report("Don't use ':' with -accel, "
> +                                 "use -M accel=... in this case instead");

s/in this case/for now/ or something like that?

Thanks,

Paolo

> +                    exit(1);
> +                }
>                  opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
>                                          false, &error_abort);
>                  qemu_opt_set(opts, "accel", optarg, &error_abort);
>