| Message ID | 1493719600-30853-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, May 02, 2017 at 12:06:40PM +0200, Thomas Huth wrote: > The '-enable-...' option do not make too much sense: They do not > allow additional parameters, using '-accel xxx' is shorter than > '-enable-xxx' and we're also inconsistent here, since there is > no '-enable-xen' option available. So let's try to convince the > users to use '-accel xxx' instead. What is our general approach to deciding which of our "legacy" parameters are merely syntactic sugar for other parameters, vs stuff we want to deprecate & eventually remove ? Regards, Daniel
On 02.05.2017 12:21, Daniel P. Berrange wrote: > On Tue, May 02, 2017 at 12:06:40PM +0200, Thomas Huth wrote: >> The '-enable-...' option do not make too much sense: They do not >> allow additional parameters, using '-accel xxx' is shorter than >> '-enable-xxx' and we're also inconsistent here, since there is >> no '-enable-xen' option available. So let's try to convince the >> users to use '-accel xxx' instead. > > What is our general approach to deciding which of our "legacy" parameters > are merely syntactic sugar for other parameters, vs stuff we want to > deprecate & eventually remove ? I'd say if the parameters can help to specify something in a short way, then it's nice-to-have syntactic sugar. Otherwise, it's just legacy cruft which should be removed one day. For the accelerator options, we've got now three (!) ways to specify them: 1) -machine accel=xxx (which is the way we use it internally, thus we should keep it) 2) -enable-xxx 3) -accel xxx -accel xxx makes sense as syntactic sugar since it helps to keep the command line short, but I fail to see the reason for the inconsistent and inflexible "-enable-xxx" here, so I'd like to suggest to mark it as deprecated so we could remove it one day (it's likely in use in the wild, so maybe remove it in v4.0.0 in a couple of years?). Thomas
On 05/02/2017 12:06 PM, Thomas Huth wrote: > The '-enable-...' option do not make too much sense: They do not > allow additional parameters, using '-accel xxx' is shorter than > '-enable-xxx' and we're also inconsistent here, since there is > no '-enable-xen' option available. So let's try to convince the > users to use '-accel xxx' instead. google has 36000 hits for "--enable-kvm" and 18000 hits for "--accel kvm" So I assume this will affect a lot of setups for only a very small benefit. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > qemu-options.hx | 5 +++-- > vl.c | 4 ++++ > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/qemu-options.hx b/qemu-options.hx > index c7b1d2d..eb33286 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3367,7 +3367,8 @@ STEXI > @item -enable-kvm > @findex -enable-kvm > Enable KVM full virtualization support. This option is only available > -if KVM support is enabled when compiling. > +if KVM support is enabled when compiling. Note: This option is deprecated, > +please use @code{-accel kvm} instead. > ETEXI > > DEF("enable-hax", 0, QEMU_OPTION_enable_hax, \ > @@ -3378,7 +3379,7 @@ STEXI > Enable HAX (Hardware-based Acceleration eXecution) support. This option > is only available if HAX support is enabled when compiling. HAX is only > applicable to MAC and Windows platform, and thus does not conflict with > -KVM. > +KVM. Note: This option is deprecated, please use @code{-accel hax} instead. > ETEXI > > DEF("xen-domid", HAS_ARG, QEMU_OPTION_xen_domid, > diff --git a/vl.c b/vl.c > index d5e88fb..d5ec87e 100644 > --- a/vl.c > +++ b/vl.c > @@ -3690,10 +3690,14 @@ int main(int argc, char **argv, char **envp) > } > break; > case QEMU_OPTION_enable_kvm: > + error_report("'-enable-kvm' is depreacted, please use " > + "'-accel kvm' instead"); > olist = qemu_find_opts("machine"); > qemu_opts_parse_noisily(olist, "accel=kvm", false); > break; > case QEMU_OPTION_enable_hax: > + error_report("'-enable-hax' is depreacted, please use " > + "'-accel hax' instead"); > olist = qemu_find_opts("machine"); > qemu_opts_parse_noisily(olist, "accel=hax", false); > break; >
On 02.05.2017 12:32, Christian Borntraeger wrote: > On 05/02/2017 12:06 PM, Thomas Huth wrote: >> The '-enable-...' option do not make too much sense: They do not >> allow additional parameters, using '-accel xxx' is shorter than >> '-enable-xxx' and we're also inconsistent here, since there is >> no '-enable-xen' option available. So let's try to convince the >> users to use '-accel xxx' instead. > > google has 36000 hits for "--enable-kvm" and 18000 hits for "--accel kvm" > So I assume this will affect a lot of setups for only a very small benefit. I'm aware of the fact that likely a lot of users are still using -enable-kvm, and I did not mean that we should remove it soon yet. But IMHO we should start now to inform the users that they should slowly switch to the better option "-accel" instead, so that we could maybe remove this "-enable-xxx" stuff sometime in the distant future (let's say QEMU v4.0?). Thomas
On 05/02/2017 12:37 PM, Thomas Huth wrote: > On 02.05.2017 12:32, Christian Borntraeger wrote: >> On 05/02/2017 12:06 PM, Thomas Huth wrote: >>> The '-enable-...' option do not make too much sense: They do not >>> allow additional parameters, using '-accel xxx' is shorter than >>> '-enable-xxx' and we're also inconsistent here, since there is >>> no '-enable-xen' option available. So let's try to convince the >>> users to use '-accel xxx' instead. >> >> google has 36000 hits for "--enable-kvm" and 18000 hits for "--accel kvm" >> So I assume this will affect a lot of setups for only a very small benefit. > > I'm aware of the fact that likely a lot of users are still using > -enable-kvm, and I did not mean that we should remove it soon yet. But > IMHO we should start now to inform the users that they should slowly > switch to the better option "-accel" instead, so that we could maybe > remove this "-enable-xxx" stuff sometime in the distant future (let's > say QEMU v4.0?). I come from the Linux side, where "breaking a working setup" will result in an angry Linus. We certainly have not such strict rules here and we could base the decision on the question "how expensive is the maintenance of this option?". I think marking it as "legacy option" is fine, but I doubt that removing it will make qemu maintenance cheaper. So my preferred variant is - have it marked in the docs as "legacy" - no error_report as it might break some setups (since error_report might write to the monitor) - never remove the option unless it turns out to be a burden But its certainly not my call to make.
On 02.05.2017 12:48, Christian Borntraeger wrote: > On 05/02/2017 12:37 PM, Thomas Huth wrote: >> On 02.05.2017 12:32, Christian Borntraeger wrote: >>> On 05/02/2017 12:06 PM, Thomas Huth wrote: >>>> The '-enable-...' option do not make too much sense: They do not >>>> allow additional parameters, using '-accel xxx' is shorter than >>>> '-enable-xxx' and we're also inconsistent here, since there is >>>> no '-enable-xen' option available. So let's try to convince the >>>> users to use '-accel xxx' instead. >>> >>> google has 36000 hits for "--enable-kvm" and 18000 hits for "--accel kvm" >>> So I assume this will affect a lot of setups for only a very small benefit. >> >> I'm aware of the fact that likely a lot of users are still using >> -enable-kvm, and I did not mean that we should remove it soon yet. But >> IMHO we should start now to inform the users that they should slowly >> switch to the better option "-accel" instead, so that we could maybe >> remove this "-enable-xxx" stuff sometime in the distant future (let's >> say QEMU v4.0?). > > I come from the Linux side, where "breaking a working setup" will result in > an angry Linus. IMHO that's a good approach, but I think it should primarily applied for the interfaces that are designed as "API" to other software layers, i.e. things like QMP and the "-machine" parameter. "-enable-kvm" is in my eyes rather a "syntactic sugar" convenience option, so I'd not apply this rule to this option. > We certainly have not such strict rules here and we could > base the decision on the question "how expensive is the maintenance > of this option?". I think marking it as "legacy option" is fine, but I doubt > that removing it will make qemu maintenance cheaper. Likely not. Actually, I have another point of view in mind here: You have to consider that QEMU has a *lot* of options, and I think this is very confusing for the users, especially the new ones. If we always provide two or three ways to achieve a goal, especially in an inconsistent way like we do it here, we likely rather create frustration than joy for the normal users. Providing a clean, straightforward CLI interface one day could help to improve the user experience quite a bit. > So my preferred variant is > - have it marked in the docs as "legacy" > - no error_report as it might break some setups (since error_report might write > to the monitor) > - never remove the option unless it turns out to be a burden > > But its certainly not my call to make. Paolo, since you're the KVM / main loop maintainer, what's your opinion here? Thomas
On Tue, May 02, 2017 at 01:26:17PM +0200, Thomas Huth wrote: > On 02.05.2017 12:48, Christian Borntraeger wrote: > > On 05/02/2017 12:37 PM, Thomas Huth wrote: > >> On 02.05.2017 12:32, Christian Borntraeger wrote: > >>> On 05/02/2017 12:06 PM, Thomas Huth wrote: > >>>> The '-enable-...' option do not make too much sense: They do not > >>>> allow additional parameters, using '-accel xxx' is shorter than > >>>> '-enable-xxx' and we're also inconsistent here, since there is > >>>> no '-enable-xen' option available. So let's try to convince the > >>>> users to use '-accel xxx' instead. > >>> > >>> google has 36000 hits for "--enable-kvm" and 18000 hits for "--accel kvm" > >>> So I assume this will affect a lot of setups for only a very small benefit. > >> > >> I'm aware of the fact that likely a lot of users are still using > >> -enable-kvm, and I did not mean that we should remove it soon yet. But > >> IMHO we should start now to inform the users that they should slowly > >> switch to the better option "-accel" instead, so that we could maybe > >> remove this "-enable-xxx" stuff sometime in the distant future (let's > >> say QEMU v4.0?). > > > > I come from the Linux side, where "breaking a working setup" will result in > > an angry Linus. > > IMHO that's a good approach, but I think it should primarily applied for > the interfaces that are designed as "API" to other software layers, i.e. > things like QMP and the "-machine" parameter. > "-enable-kvm" is in my eyes rather a "syntactic sugar" convenience > option, so I'd not apply this rule to this option. > > > We certainly have not such strict rules here and we could > > base the decision on the question "how expensive is the maintenance > > of this option?". I think marking it as "legacy option" is fine, but I doubt > > that removing it will make qemu maintenance cheaper. > > Likely not. Actually, I have another point of view in mind here: You > have to consider that QEMU has a *lot* of options, and I think this is > very confusing for the users, especially the new ones. If we always > provide two or three ways to achieve a goal, especially in an > inconsistent way like we do it here, we likely rather create frustration > than joy for the normal users. Providing a clean, straightforward CLI > interface one day could help to improve the user experience quite a bit. The issue is that we have mutually exclusive requirements here. For a straightforward, easy to understand CLI, things like "--enable-kvm" are much quicker to discover & understand than "-machine accel=kvm". The latter gives much more flexibility since it can set all the other opts, but most of those are rarely used by people who are invoking QEMU manually/directly. We need the things like -machine for libvirt and similar, but they are not end user friendly. Killing all the shortcuts like --enable-kvm would cut down the args we expose, but forcing users onto more complex syntax for args like -machine is not improving their lives in general if they don't need that extra flexibility. Regards, Daniel
On 02.05.2017 13:59, Daniel P. Berrange wrote: > On Tue, May 02, 2017 at 01:26:17PM +0200, Thomas Huth wrote: >> On 02.05.2017 12:48, Christian Borntraeger wrote: >>> On 05/02/2017 12:37 PM, Thomas Huth wrote: >>>> On 02.05.2017 12:32, Christian Borntraeger wrote: >>>>> On 05/02/2017 12:06 PM, Thomas Huth wrote: >>>>>> The '-enable-...' option do not make too much sense: They do not >>>>>> allow additional parameters, using '-accel xxx' is shorter than >>>>>> '-enable-xxx' and we're also inconsistent here, since there is >>>>>> no '-enable-xen' option available. So let's try to convince the >>>>>> users to use '-accel xxx' instead. >>>>> >>>>> google has 36000 hits for "--enable-kvm" and 18000 hits for "--accel kvm" >>>>> So I assume this will affect a lot of setups for only a very small benefit. >>>> >>>> I'm aware of the fact that likely a lot of users are still using >>>> -enable-kvm, and I did not mean that we should remove it soon yet. But >>>> IMHO we should start now to inform the users that they should slowly >>>> switch to the better option "-accel" instead, so that we could maybe >>>> remove this "-enable-xxx" stuff sometime in the distant future (let's >>>> say QEMU v4.0?). >>> >>> I come from the Linux side, where "breaking a working setup" will result in >>> an angry Linus. >> >> IMHO that's a good approach, but I think it should primarily applied for >> the interfaces that are designed as "API" to other software layers, i.e. >> things like QMP and the "-machine" parameter. >> "-enable-kvm" is in my eyes rather a "syntactic sugar" convenience >> option, so I'd not apply this rule to this option. >> >>> We certainly have not such strict rules here and we could >>> base the decision on the question "how expensive is the maintenance >>> of this option?". I think marking it as "legacy option" is fine, but I doubt >>> that removing it will make qemu maintenance cheaper. >> >> Likely not. Actually, I have another point of view in mind here: You >> have to consider that QEMU has a *lot* of options, and I think this is >> very confusing for the users, especially the new ones. If we always >> provide two or three ways to achieve a goal, especially in an >> inconsistent way like we do it here, we likely rather create frustration >> than joy for the normal users. Providing a clean, straightforward CLI >> interface one day could help to improve the user experience quite a bit. > > The issue is that we have mutually exclusive requirements here. For a > straightforward, easy to understand CLI, things like "--enable-kvm" are > much quicker to discover & understand than "-machine accel=kvm". The > latter gives much more flexibility since it can set all the other opts, > but most of those are rarely used by people who are invoking QEMU > manually/directly. We need the things like -machine for libvirt and > similar, but they are not end user friendly. Killing all the shortcuts > like --enable-kvm would cut down the args we expose, but forcing users > onto more complex syntax for args like -machine is not improving their > lives in general if they don't need that extra flexibility. Theoretically yes, but in this case we also have the "-accel kvm" option which is IMHO also straighforward and easy to understand, and even shorter than "-enable-kvm". If you look at my patch, I did *not* want to force the normal users to use "-machine accel=kvm" here, so I don't see the point of your argument here. Thomas
On 02.05.2017 14:07, Thomas Huth wrote: > On 02.05.2017 13:59, Daniel P. Berrange wrote: >> On Tue, May 02, 2017 at 01:26:17PM +0200, Thomas Huth wrote: >>> On 02.05.2017 12:48, Christian Borntraeger wrote: >>>> On 05/02/2017 12:37 PM, Thomas Huth wrote: >>>>> On 02.05.2017 12:32, Christian Borntraeger wrote: >>>>>> On 05/02/2017 12:06 PM, Thomas Huth wrote: >>>>>>> The '-enable-...' option do not make too much sense: They do not >>>>>>> allow additional parameters, using '-accel xxx' is shorter than >>>>>>> '-enable-xxx' and we're also inconsistent here, since there is >>>>>>> no '-enable-xen' option available. So let's try to convince the >>>>>>> users to use '-accel xxx' instead. >>>>>> >>>>>> google has 36000 hits for "--enable-kvm" and 18000 hits for "--accel kvm" >>>>>> So I assume this will affect a lot of setups for only a very small benefit. >>>>> >>>>> I'm aware of the fact that likely a lot of users are still using >>>>> -enable-kvm, and I did not mean that we should remove it soon yet. But >>>>> IMHO we should start now to inform the users that they should slowly >>>>> switch to the better option "-accel" instead, so that we could maybe >>>>> remove this "-enable-xxx" stuff sometime in the distant future (let's >>>>> say QEMU v4.0?). >>>> >>>> I come from the Linux side, where "breaking a working setup" will result in >>>> an angry Linus. >>> >>> IMHO that's a good approach, but I think it should primarily applied for >>> the interfaces that are designed as "API" to other software layers, i.e. >>> things like QMP and the "-machine" parameter. >>> "-enable-kvm" is in my eyes rather a "syntactic sugar" convenience >>> option, so I'd not apply this rule to this option. >>> >>>> We certainly have not such strict rules here and we could >>>> base the decision on the question "how expensive is the maintenance >>>> of this option?". I think marking it as "legacy option" is fine, but I doubt >>>> that removing it will make qemu maintenance cheaper. >>> >>> Likely not. Actually, I have another point of view in mind here: You >>> have to consider that QEMU has a *lot* of options, and I think this is >>> very confusing for the users, especially the new ones. If we always >>> provide two or three ways to achieve a goal, especially in an >>> inconsistent way like we do it here, we likely rather create frustration >>> than joy for the normal users. Providing a clean, straightforward CLI >>> interface one day could help to improve the user experience quite a bit. >> >> The issue is that we have mutually exclusive requirements here. For a >> straightforward, easy to understand CLI, things like "--enable-kvm" are >> much quicker to discover & understand than "-machine accel=kvm". The >> latter gives much more flexibility since it can set all the other opts, >> but most of those are rarely used by people who are invoking QEMU >> manually/directly. We need the things like -machine for libvirt and >> similar, but they are not end user friendly. Killing all the shortcuts >> like --enable-kvm would cut down the args we expose, but forcing users >> onto more complex syntax for args like -machine is not improving their >> lives in general if they don't need that extra flexibility. > > Theoretically yes, but in this case we also have the "-accel kvm" option > which is IMHO also straighforward and easy to understand, and even > shorter than "-enable-kvm". If you look at my patch, I did *not* want to > force the normal users to use "-machine accel=kvm" here, so I don't see > the point of your argument here. Apart from that, we never use "-enable-xxx" for any other convenience option, e.g. we do not use "-enable-usb", but just "-usb". So for a real convenience option to enable KVM, the option should have been simply named "-kvm" instead. That "-enable-xxx" stuff is just bad and IMHO does not really fit into the QEMU world. Thomas
On Tue, May 02, 2017 at 02:07:15PM +0200, Thomas Huth wrote: > On 02.05.2017 13:59, Daniel P. Berrange wrote: > > On Tue, May 02, 2017 at 01:26:17PM +0200, Thomas Huth wrote: > >> On 02.05.2017 12:48, Christian Borntraeger wrote: > >>> On 05/02/2017 12:37 PM, Thomas Huth wrote: > >>>> On 02.05.2017 12:32, Christian Borntraeger wrote: > >>>>> On 05/02/2017 12:06 PM, Thomas Huth wrote: > >>>>>> The '-enable-...' option do not make too much sense: They do not > >>>>>> allow additional parameters, using '-accel xxx' is shorter than > >>>>>> '-enable-xxx' and we're also inconsistent here, since there is > >>>>>> no '-enable-xen' option available. So let's try to convince the > >>>>>> users to use '-accel xxx' instead. > >>>>> > >>>>> google has 36000 hits for "--enable-kvm" and 18000 hits for "--accel kvm" > >>>>> So I assume this will affect a lot of setups for only a very small benefit. > >>>> > >>>> I'm aware of the fact that likely a lot of users are still using > >>>> -enable-kvm, and I did not mean that we should remove it soon yet. But > >>>> IMHO we should start now to inform the users that they should slowly > >>>> switch to the better option "-accel" instead, so that we could maybe > >>>> remove this "-enable-xxx" stuff sometime in the distant future (let's > >>>> say QEMU v4.0?). > >>> > >>> I come from the Linux side, where "breaking a working setup" will result in > >>> an angry Linus. > >> > >> IMHO that's a good approach, but I think it should primarily applied for > >> the interfaces that are designed as "API" to other software layers, i.e. > >> things like QMP and the "-machine" parameter. > >> "-enable-kvm" is in my eyes rather a "syntactic sugar" convenience > >> option, so I'd not apply this rule to this option. > >> > >>> We certainly have not such strict rules here and we could > >>> base the decision on the question "how expensive is the maintenance > >>> of this option?". I think marking it as "legacy option" is fine, but I doubt > >>> that removing it will make qemu maintenance cheaper. > >> > >> Likely not. Actually, I have another point of view in mind here: You > >> have to consider that QEMU has a *lot* of options, and I think this is > >> very confusing for the users, especially the new ones. If we always > >> provide two or three ways to achieve a goal, especially in an > >> inconsistent way like we do it here, we likely rather create frustration > >> than joy for the normal users. Providing a clean, straightforward CLI > >> interface one day could help to improve the user experience quite a bit. > > > > The issue is that we have mutually exclusive requirements here. For a > > straightforward, easy to understand CLI, things like "--enable-kvm" are > > much quicker to discover & understand than "-machine accel=kvm". The > > latter gives much more flexibility since it can set all the other opts, > > but most of those are rarely used by people who are invoking QEMU > > manually/directly. We need the things like -machine for libvirt and > > similar, but they are not end user friendly. Killing all the shortcuts > > like --enable-kvm would cut down the args we expose, but forcing users > > onto more complex syntax for args like -machine is not improving their > > lives in general if they don't need that extra flexibility. > > Theoretically yes, but in this case we also have the "-accel kvm" option > which is IMHO also straighforward and easy to understand, and even > shorter than "-enable-kvm". If you look at my patch, I did *not* want to > force the normal users to use "-machine accel=kvm" here, so I don't see > the point of your argument here. Just looking at the -help output though we have "-machine [type=]name[,prop[=value][,...]] property accel=accel1[:accel2[:...]] selects accelerator supported accelerators are kvm, xen, tcg (default: tcg)" and "-enable-kvm enable KVM full virtualization support" but the "-accel" option is not documented, so essentially doesn't exist, unless you read the man page to see if there are other secret options listed there. So from -help output, "-enable-kvm" looks like the best option to pick from a novice user POV. Regards, Daniel
On 05/02/2017 12:37 PM, Thomas Huth wrote: > On 02.05.2017 12:32, Christian Borntraeger wrote: >> On 05/02/2017 12:06 PM, Thomas Huth wrote: >>> The '-enable-...' option do not make too much sense: They do not >>> allow additional parameters, using '-accel xxx' is shorter than >>> '-enable-xxx' and we're also inconsistent here, since there is >>> no '-enable-xen' option available. So let's try to convince the >>> users to use '-accel xxx' instead. FWIW, -enable-kvm ia not shortcut to make things easier, it predates the accel parameter. Maybe that is the reason why this patch does not feel right to me. >> >> google has 36000 hits for "--enable-kvm" and 18000 hits for "--accel kvm" >> So I assume this will affect a lot of setups for only a very small benefit. > > I'm aware of the fact that likely a lot of users are still using > -enable-kvm, and I did not mean that we should remove it soon yet. But > IMHO we should start now to inform the users that they should slowly > switch to the better option "-accel" instead, so that we could maybe > remove this "-enable-xxx" stuff sometime in the distant future (let's > say QEMU v4.0?). > > Thomas >
On Tue, May 02, 2017 at 01:16:33PM +0100, Daniel P. Berrange wrote: > On Tue, May 02, 2017 at 02:07:15PM +0200, Thomas Huth wrote: > > On 02.05.2017 13:59, Daniel P. Berrange wrote: > > > On Tue, May 02, 2017 at 01:26:17PM +0200, Thomas Huth wrote: > > >> On 02.05.2017 12:48, Christian Borntraeger wrote: > > >>> On 05/02/2017 12:37 PM, Thomas Huth wrote: > > >>>> On 02.05.2017 12:32, Christian Borntraeger wrote: > > >>>>> On 05/02/2017 12:06 PM, Thomas Huth wrote: > > >>>>>> The '-enable-...' option do not make too much sense: They do not > > >>>>>> allow additional parameters, using '-accel xxx' is shorter than > > >>>>>> '-enable-xxx' and we're also inconsistent here, since there is > > >>>>>> no '-enable-xen' option available. So let's try to convince the > > >>>>>> users to use '-accel xxx' instead. > > >>>>> > > >>>>> google has 36000 hits for "--enable-kvm" and 18000 hits for "--accel kvm" > > >>>>> So I assume this will affect a lot of setups for only a very small benefit. > > >>>> > > >>>> I'm aware of the fact that likely a lot of users are still using > > >>>> -enable-kvm, and I did not mean that we should remove it soon yet. But > > >>>> IMHO we should start now to inform the users that they should slowly > > >>>> switch to the better option "-accel" instead, so that we could maybe > > >>>> remove this "-enable-xxx" stuff sometime in the distant future (let's > > >>>> say QEMU v4.0?). > > >>> > > >>> I come from the Linux side, where "breaking a working setup" will result in > > >>> an angry Linus. > > >> > > >> IMHO that's a good approach, but I think it should primarily applied for > > >> the interfaces that are designed as "API" to other software layers, i.e. > > >> things like QMP and the "-machine" parameter. > > >> "-enable-kvm" is in my eyes rather a "syntactic sugar" convenience > > >> option, so I'd not apply this rule to this option. > > >> > > >>> We certainly have not such strict rules here and we could > > >>> base the decision on the question "how expensive is the maintenance > > >>> of this option?". I think marking it as "legacy option" is fine, but I doubt > > >>> that removing it will make qemu maintenance cheaper. > > >> > > >> Likely not. Actually, I have another point of view in mind here: You > > >> have to consider that QEMU has a *lot* of options, and I think this is > > >> very confusing for the users, especially the new ones. If we always > > >> provide two or three ways to achieve a goal, especially in an > > >> inconsistent way like we do it here, we likely rather create frustration > > >> than joy for the normal users. Providing a clean, straightforward CLI > > >> interface one day could help to improve the user experience quite a bit. > > > > > > The issue is that we have mutually exclusive requirements here. For a > > > straightforward, easy to understand CLI, things like "--enable-kvm" are > > > much quicker to discover & understand than "-machine accel=kvm". The > > > latter gives much more flexibility since it can set all the other opts, > > > but most of those are rarely used by people who are invoking QEMU > > > manually/directly. We need the things like -machine for libvirt and > > > similar, but they are not end user friendly. Killing all the shortcuts > > > like --enable-kvm would cut down the args we expose, but forcing users > > > onto more complex syntax for args like -machine is not improving their > > > lives in general if they don't need that extra flexibility. > > > > Theoretically yes, but in this case we also have the "-accel kvm" option > > which is IMHO also straighforward and easy to understand, and even > > shorter than "-enable-kvm". If you look at my patch, I did *not* want to > > force the normal users to use "-machine accel=kvm" here, so I don't see > > the point of your argument here. > > Just looking at the -help output though we have > > "-machine [type=]name[,prop[=value][,...]] > property accel=accel1[:accel2[:...]] selects accelerator > supported accelerators are kvm, xen, tcg (default: tcg)" > > and > > "-enable-kvm enable KVM full virtualization support" > > but the "-accel" option is not documented, so essentially doesn't exist, > unless you read the man page to see if there are other secret options > listed there. So from -help output, "-enable-kvm" looks like the best > option to pick from a novice user POV. Opps, I was looking at wrong QEMU binary - it is listed "-accel [accel=]accelerator[,thread=single|multi] select accelerator ('-accel help for list')" but it doesn't mention KVM, so I still think -enable-kvm is the more obvious right answer based on what we're telling users in help Regards, Daniel
On 02.05.2017 14:38, Daniel P. Berrange wrote: > On Tue, May 02, 2017 at 01:16:33PM +0100, Daniel P. Berrange wrote: >> On Tue, May 02, 2017 at 02:07:15PM +0200, Thomas Huth wrote: >>> On 02.05.2017 13:59, Daniel P. Berrange wrote: >>>> On Tue, May 02, 2017 at 01:26:17PM +0200, Thomas Huth wrote: >>>>> On 02.05.2017 12:48, Christian Borntraeger wrote: >>>>>> On 05/02/2017 12:37 PM, Thomas Huth wrote: >>>>>>> On 02.05.2017 12:32, Christian Borntraeger wrote: >>>>>>>> On 05/02/2017 12:06 PM, Thomas Huth wrote: >>>>>>>>> The '-enable-...' option do not make too much sense: They do not >>>>>>>>> allow additional parameters, using '-accel xxx' is shorter than >>>>>>>>> '-enable-xxx' and we're also inconsistent here, since there is >>>>>>>>> no '-enable-xen' option available. So let's try to convince the >>>>>>>>> users to use '-accel xxx' instead. >>>>>>>> >>>>>>>> google has 36000 hits for "--enable-kvm" and 18000 hits for "--accel kvm" >>>>>>>> So I assume this will affect a lot of setups for only a very small benefit. >>>>>>> >>>>>>> I'm aware of the fact that likely a lot of users are still using >>>>>>> -enable-kvm, and I did not mean that we should remove it soon yet. But >>>>>>> IMHO we should start now to inform the users that they should slowly >>>>>>> switch to the better option "-accel" instead, so that we could maybe >>>>>>> remove this "-enable-xxx" stuff sometime in the distant future (let's >>>>>>> say QEMU v4.0?). >>>>>> >>>>>> I come from the Linux side, where "breaking a working setup" will result in >>>>>> an angry Linus. >>>>> >>>>> IMHO that's a good approach, but I think it should primarily applied for >>>>> the interfaces that are designed as "API" to other software layers, i.e. >>>>> things like QMP and the "-machine" parameter. >>>>> "-enable-kvm" is in my eyes rather a "syntactic sugar" convenience >>>>> option, so I'd not apply this rule to this option. >>>>> >>>>>> We certainly have not such strict rules here and we could >>>>>> base the decision on the question "how expensive is the maintenance >>>>>> of this option?". I think marking it as "legacy option" is fine, but I doubt >>>>>> that removing it will make qemu maintenance cheaper. >>>>> >>>>> Likely not. Actually, I have another point of view in mind here: You >>>>> have to consider that QEMU has a *lot* of options, and I think this is >>>>> very confusing for the users, especially the new ones. If we always >>>>> provide two or three ways to achieve a goal, especially in an >>>>> inconsistent way like we do it here, we likely rather create frustration >>>>> than joy for the normal users. Providing a clean, straightforward CLI >>>>> interface one day could help to improve the user experience quite a bit. >>>> >>>> The issue is that we have mutually exclusive requirements here. For a >>>> straightforward, easy to understand CLI, things like "--enable-kvm" are >>>> much quicker to discover & understand than "-machine accel=kvm". The >>>> latter gives much more flexibility since it can set all the other opts, >>>> but most of those are rarely used by people who are invoking QEMU >>>> manually/directly. We need the things like -machine for libvirt and >>>> similar, but they are not end user friendly. Killing all the shortcuts >>>> like --enable-kvm would cut down the args we expose, but forcing users >>>> onto more complex syntax for args like -machine is not improving their >>>> lives in general if they don't need that extra flexibility. >>> >>> Theoretically yes, but in this case we also have the "-accel kvm" option >>> which is IMHO also straighforward and easy to understand, and even >>> shorter than "-enable-kvm". If you look at my patch, I did *not* want to >>> force the normal users to use "-machine accel=kvm" here, so I don't see >>> the point of your argument here. >> >> Just looking at the -help output though we have >> >> "-machine [type=]name[,prop[=value][,...]] >> property accel=accel1[:accel2[:...]] selects accelerator >> supported accelerators are kvm, xen, tcg (default: tcg)" >> >> and >> >> "-enable-kvm enable KVM full virtualization support" >> >> but the "-accel" option is not documented, so essentially doesn't exist, >> unless you read the man page to see if there are other secret options >> listed there. So from -help output, "-enable-kvm" looks like the best >> option to pick from a novice user POV. > > Opps, I was looking at wrong QEMU binary - it is listed > > "-accel [accel=]accelerator[,thread=single|multi] > select accelerator ('-accel help for list')" > > but it doesn't mention KVM, so I still think -enable-kvm is the more obvious > right answer based on what we're telling users in help Oh, right, that parameter has just been introduced with QEMU 2.9.0 ... so maybe it's really too early to spill out deprecation messages for "-enable-kvm", and we just should update the documentation instead (as Christian suggested)... Thomas
On 02/05/2017 12:48, Christian Borntraeger wrote: >> I'm aware of the fact that likely a lot of users are still using >> -enable-kvm, and I did not mean that we should remove it soon yet. But >> IMHO we should start now to inform the users that they should slowly >> switch to the better option "-accel" instead, so that we could maybe >> remove this "-enable-xxx" stuff sometime in the distant future (let's >> say QEMU v4.0?). > > I come from the Linux side, where "breaking a working setup" will result in > an angry Linus. We certainly have not such strict rules here and we could > base the decision on the question "how expensive is the maintenance > of this option?". I think marking it as "legacy option" is fine, but I doubt > that removing it will make qemu maintenance cheaper. So my preferred variant > is Even for Linux the rules aren't 100% black or white. There have been cases where small breakage are introduced, for example 4.12 will break the BLKDISCARDZEROES ioctl. We shouldn't treat things are black or white here too. "-usbdevice" makes sense to deprecate because it brings together a complicated parsing mechanism, though perhaps we could keep it for simple devices such as keyboard, mouse, tablet where it's pretty surely in use in the wild. "-drive cyls=..." also makes sense to deprecate because it is a very obscure functionality. "-drive serial=..." would be nice to deprecate, but I think it's already less clear that it's not in use in the wild. Probably it's still on the side of wanting to eventually remove it. "-net" is more complicated still. It's almost surely in use in the wild, but then probably the users are fairly advanced and (with the provision that documentation should be updated) I guess we could eventually get there. But I really, really see no point in removing --enable-kvm. It costs perhaps 10 lines of code that has pretty much no ramifications elsewhere in the code. If anything I'd expect "-machine accel" to disappear before, if ever (in favor of multiple "-accel" options, e.g. "-machine accel=kvm:tcg" can become "-accel kvm -accel tcg". Thanks, Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 02/05/2017 12:48, Christian Borntraeger wrote: >>> I'm aware of the fact that likely a lot of users are still using >>> -enable-kvm, and I did not mean that we should remove it soon yet. But >>> IMHO we should start now to inform the users that they should slowly >>> switch to the better option "-accel" instead, so that we could maybe >>> remove this "-enable-xxx" stuff sometime in the distant future (let's >>> say QEMU v4.0?). >> >> I come from the Linux side, where "breaking a working setup" will result in >> an angry Linus. We certainly have not such strict rules here and we could >> base the decision on the question "how expensive is the maintenance >> of this option?". I think marking it as "legacy option" is fine, but I doubt >> that removing it will make qemu maintenance cheaper. So my preferred variant >> is > > Even for Linux the rules aren't 100% black or white. There have been > cases where small breakage are introduced, for example 4.12 will break > the BLKDISCARDZEROES ioctl. > > We shouldn't treat things are black or white here too. > > "-usbdevice" makes sense to deprecate because it brings together a > complicated parsing mechanism, though perhaps we could keep it for > simple devices such as keyboard, mouse, tablet where it's pretty surely > in use in the wild. > > "-drive cyls=..." also makes sense to deprecate because it is a very > obscure functionality. > > "-drive serial=..." would be nice to deprecate, but I think it's already > less clear that it's not in use in the wild. Probably it's still on the > side of wanting to eventually remove it. > > "-net" is more complicated still. It's almost surely in use in the > wild, but then probably the users are fairly advanced and (with the > provision that documentation should be updated) I guess we could > eventually get there. > > But I really, really see no point in removing --enable-kvm. It costs > perhaps 10 lines of code that has pretty much no ramifications elsewhere > in the code. If anything I'd expect "-machine accel" to disappear > before, if ever (in favor of multiple "-accel" options, e.g. "-machine > accel=kvm:tcg" can become "-accel kvm -accel tcg". I basically agree, except I wouldn't say "no point", but "doesn't buy us enough to justify inconveniencing users". What we've done before for options that have become unloved, but not harmful, is remove them just from documentation. Grep qemu-options.hx for "HXCOMM Deprecated". Let's do that for --enable-kvm and similarly weird sugared options.
On 02/05/2017 17:01, Markus Armbruster wrote: >> But I really, really see no point in removing --enable-kvm. It costs >> perhaps 10 lines of code that has pretty much no ramifications elsewhere >> in the code. If anything I'd expect "-machine accel" to disappear >> before, if ever (in favor of multiple "-accel" options, e.g. "-machine >> accel=kvm:tcg" can become "-accel kvm -accel tcg". > I basically agree, except I wouldn't say "no point", but "doesn't buy us > enough to justify inconveniencing users". > > What we've done before for options that have become unloved, but not > harmful, is remove them just from documentation. Grep qemu-options.hx > for "HXCOMM Deprecated". Let's do that for --enable-kvm and similarly > weird sugared options. Except that this provides no easily greppable way to find how to enable KVM, as things stand. So the first lesson should be "no deprecation without documentation" (any reference to 18th century political slogans is purely coincidential :)). Paolo
On 02.05.2017 17:09, Paolo Bonzini wrote: > > > On 02/05/2017 17:01, Markus Armbruster wrote: >>> But I really, really see no point in removing --enable-kvm. It costs >>> perhaps 10 lines of code that has pretty much no ramifications elsewhere >>> in the code. If anything I'd expect "-machine accel" to disappear >>> before, if ever (in favor of multiple "-accel" options, e.g. "-machine >>> accel=kvm:tcg" can become "-accel kvm -accel tcg". >> I basically agree, except I wouldn't say "no point", but "doesn't buy us >> enough to justify inconveniencing users". >> >> What we've done before for options that have become unloved, but not >> harmful, is remove them just from documentation. Grep qemu-options.hx >> for "HXCOMM Deprecated". Let's do that for --enable-kvm and similarly >> weird sugared options. > > Except that this provides no easily greppable way to find how to enable > KVM, as things stand. Should we then improve the description of the -accel parameter? > So the first lesson should be "no deprecation > without documentation" (any reference to 18th century political slogans > is purely coincidential :)). Would you accept my patch if I'd remove the hunks for vl.c, i.e. a patch that just updates the documentation? Thomas PS: Thinking about all of this again, I think the thing that bugs me most is that we now have a brand new "-enable-hax" option, too, but no "-enable-xen" option ... that's just inconsistent. What do you think about removing the "-enable-hax" option again - there should only be very few users for this option so far, so the impact should be low. HAX users could simply use the "-accel hax" instead...
On 02/05/2017 17:53, Thomas Huth wrote: >>> >>> What we've done before for options that have become unloved, but not >>> harmful, is remove them just from documentation. Grep qemu-options.hx >>> for "HXCOMM Deprecated". Let's do that for --enable-kvm and similarly >>> weird sugared options. >> Except that this provides no easily greppable way to find how to enable >> KVM, as things stand. > Should we then improve the description of the -accel parameter? Yes, having a list of accelerators would be useful. For example: -accel tcg[,threads=on|off] .... -accel kvm -accel hax -accel qtest >> So the first lesson should be "no deprecation >> without documentation" (any reference to 18th century political slogans >> is purely coincidential :)). > Would you accept my patch if I'd remove the hunks for vl.c, i.e. a patch > that just updates the documentation? No, updating the documentation is pointless if you don't improve it at the same time. I don't like writing it so often, especially to someone who is a relatively experienced contributor, but the common word in all these reviews is "pointless". What is the *purpose* of this work? I think a valid purpose would be to improve documentation, for example. Another would be to consolidate non-QemuOpts command-line options. For this, some options that are entirely internal provide interesting grounds for improvement. For example, "-qtest" and "qtest-log" could be consolidated in "-qtest [chardev=]chardev,log=file". Paolo
On 02.05.2017 18:08, Paolo Bonzini wrote: [...] >>> So the first lesson should be "no deprecation >>> without documentation" (any reference to 18th century political slogans >>> is purely coincidential :)). >> Would you accept my patch if I'd remove the hunks for vl.c, i.e. a patch >> that just updates the documentation? > > No, updating the documentation is pointless if you don't improve it at > the same time. > > I don't like writing it so often, especially to someone who is a > relatively experienced contributor, but the common word in all these > reviews is "pointless". What is the *purpose* of this work? Marking the -enable-xxx options in the documentation as deprecated would maybe at least prevent that we end up with more -enable-mynewcoolaccelerator options in the future, like we did with -enable-hax now. But ok, I got the message, and I'll shut up here now. Sorry for wasting your time. Thomas
On 02/05/2017 18:19, Thomas Huth wrote: >> I don't like writing it so often, especially to someone who is a >> relatively experienced contributor, but the common word in all these >> reviews is "pointless". What is the *purpose* of this work? > > Marking the -enable-xxx options in the documentation as deprecated would > maybe at least prevent that we end up with more > -enable-mynewcoolaccelerator options in the future, like we did with > -enable-hax now. That assumes -enable-hax was a mistake. :) > But ok, I got the message, and I'll shut up here now. Sorry for wasting > your time. That absolutely wasn't the intent, sorry. The intent is to make your efforts more useful. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 02/05/2017 18:19, Thomas Huth wrote: >>> I don't like writing it so often, especially to someone who is a >>> relatively experienced contributor, but the common word in all these >>> reviews is "pointless". What is the *purpose* of this work? >> >> Marking the -enable-xxx options in the documentation as deprecated would >> maybe at least prevent that we end up with more >> -enable-mynewcoolaccelerator options in the future, like we did with >> -enable-hax now. > > That assumes -enable-hax was a mistake. :) It was, or else -accel was a mistake. >> But ok, I got the message, and I'll shut up here now. Sorry for wasting >> your time. > > That absolutely wasn't the intent, sorry. The intent is to make your > efforts more useful. Seconded. I hope your patches and our discussion lead us towards a saner, more consistent documented public interface.
diff --git a/qemu-options.hx b/qemu-options.hx index c7b1d2d..eb33286 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3367,7 +3367,8 @@ STEXI @item -enable-kvm @findex -enable-kvm Enable KVM full virtualization support. This option is only available -if KVM support is enabled when compiling. +if KVM support is enabled when compiling. Note: This option is deprecated, +please use @code{-accel kvm} instead. ETEXI DEF("enable-hax", 0, QEMU_OPTION_enable_hax, \ @@ -3378,7 +3379,7 @@ STEXI Enable HAX (Hardware-based Acceleration eXecution) support. This option is only available if HAX support is enabled when compiling. HAX is only applicable to MAC and Windows platform, and thus does not conflict with -KVM. +KVM. Note: This option is deprecated, please use @code{-accel hax} instead. ETEXI DEF("xen-domid", HAS_ARG, QEMU_OPTION_xen_domid, diff --git a/vl.c b/vl.c index d5e88fb..d5ec87e 100644 --- a/vl.c +++ b/vl.c @@ -3690,10 +3690,14 @@ int main(int argc, char **argv, char **envp) } break; case QEMU_OPTION_enable_kvm: + error_report("'-enable-kvm' is depreacted, please use " + "'-accel kvm' instead"); olist = qemu_find_opts("machine"); qemu_opts_parse_noisily(olist, "accel=kvm", false); break; case QEMU_OPTION_enable_hax: + error_report("'-enable-hax' is depreacted, please use " + "'-accel hax' instead"); olist = qemu_find_opts("machine"); qemu_opts_parse_noisily(olist, "accel=hax", false); break;
The '-enable-...' option do not make too much sense: They do not allow additional parameters, using '-accel xxx' is shorter than '-enable-xxx' and we're also inconsistent here, since there is no '-enable-xen' option available. So let's try to convince the users to use '-accel xxx' instead. Signed-off-by: Thomas Huth <thuth@redhat.com> --- qemu-options.hx | 5 +++-- vl.c | 4 ++++ 2 files changed, 7 insertions(+), 2 deletions(-)