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

[Thomas Huth <thuth@redhat.com>]

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