| Message ID | 1499049848-18012-4-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
Peter Xu <peterx@redhat.com> wrote: > It's never documented, and now we have one more parameter for it (which > means this one can be obsolete in the future). Document it properly. > > Although now when enforce-config-section is set, it'll override the > other "-global" parameter, that is not necessarily a rule. Forbid that > usage in the document. > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > qemu-options.hx | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/qemu-options.hx b/qemu-options.hx > index 297bd8a..927c51f 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off. > @item s390-squash-mcss=on|off > Enables or disables squashing subchannels into the default css. > The default is off. > +@item enforce-config-section=on|off > +Decides whether we will send the configuration section when doing > +migration. By default, it is turned on. We can set this to off to > +explicitly disable it. Note: this parameter will be obsolete soon, > +please use "-global migration.send-configuration=on|off" instead. > +"enforce-config-section" cannot be used together with "-global > +migration.send-configuration". If it happens, the behavior is > +undefined. > @end table > ETEXI Reviewed-by: Juan Quintela <quintela@redhat.com> I will have been explicit that it is *already* obsolete, just that we don't have a way to say that. Later, Juan.
On Mon, Jul 03, 2017 at 10:44:07AM +0800, Peter Xu wrote: > It's never documented, and now we have one more parameter for it (which > means this one can be obsolete in the future). Document it properly. > > Although now when enforce-config-section is set, it'll override the > other "-global" parameter, that is not necessarily a rule. Forbid that > usage in the document. > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > qemu-options.hx | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/qemu-options.hx b/qemu-options.hx > index 297bd8a..927c51f 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off. > @item s390-squash-mcss=on|off > Enables or disables squashing subchannels into the default css. > The default is off. > +@item enforce-config-section=on|off > +Decides whether we will send the configuration section when doing > +migration. By default, it is turned on. We can set this to off to > +explicitly disable it. [...] Wait, isn't it off by default? This seems to imply that "-machine enforce-config-section=on" would have no effect at all, as the option would be already on by default. This is not the case. I suggest rewriting this as: If set to "on, force migration code to send configuration section even if the machine-type sets the "migration.send-configuration" property to "off". Note: this parameter is obsolete, please use "-global migration.send-configuration=on|off" instead. Behavior is undefined if "enforce-config-section" and "-global migration.send-configuration" are used together. (Note: we probably should use proper markup (@option/@var/@samp?) instead of quotes above, to format the option names properly in the generated documentation.)
On Mon, Jul 03, 2017 at 02:07:12PM -0300, Eduardo Habkost wrote: > On Mon, Jul 03, 2017 at 10:44:07AM +0800, Peter Xu wrote: > > It's never documented, and now we have one more parameter for it (which > > means this one can be obsolete in the future). Document it properly. > > > > Although now when enforce-config-section is set, it'll override the > > other "-global" parameter, that is not necessarily a rule. Forbid that > > usage in the document. > > > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > qemu-options.hx | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/qemu-options.hx b/qemu-options.hx > > index 297bd8a..927c51f 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off. > > @item s390-squash-mcss=on|off > > Enables or disables squashing subchannels into the default css. > > The default is off. > > +@item enforce-config-section=on|off > > +Decides whether we will send the configuration section when doing > > +migration. By default, it is turned on. We can set this to off to > > +explicitly disable it. > [...] > > Wait, isn't it off by default? > > This seems to imply that "-machine enforce-config-section=on" would have > no effect at all, as the option would be already on by default. This is > not the case. > > I suggest rewriting this as: > > If set to "on, force migration code to send configuration section even > if the machine-type sets the "migration.send-configuration" property > to "off". > Note: this parameter is obsolete, please use "-global > migration.send-configuration=on|off" instead. > Behavior is undefined if "enforce-config-section" and "-global > migration.send-configuration" are used together. > > (Note: we probably should use proper markup (@option/@var/@samp?) > instead of quotes above, to format the option names properly in the > generated documentation.) Yes, you are right. How's this one? (markup used this time) If @option{enforce-config-section} is set to @var{on}, force migration code to send configuration section even if the machine-type sets the @option{migration.send-configuration} property to @var{off}. @option{enforce-config-section} cannot be used together with @option{-global} @option{migration.send-configuration}. Behavior is undefined if @option{enforce-config-section} and @option{-global} @option{migration.send-configuration} are used together. NOTE: this parameter is obsolete. Please use @option{-global} @option{migration.send-configuration}=@var{on|off} instead. Thanks,
Peter Xu <peterx@redhat.com> writes: > It's never documented, and now we have one more parameter for it (which > means this one can be obsolete in the future). Document it properly. > > Although now when enforce-config-section is set, it'll override the > other "-global" parameter, that is not necessarily a rule. Forbid that > usage in the document. > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > qemu-options.hx | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/qemu-options.hx b/qemu-options.hx > index 297bd8a..927c51f 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off. > @item s390-squash-mcss=on|off > Enables or disables squashing subchannels into the default css. > The default is off. > +@item enforce-config-section=on|off > +Decides whether we will send the configuration section when doing > +migration. By default, it is turned on. We can set this to off to > +explicitly disable it. Note: this parameter will be obsolete soon, > +please use "-global migration.send-configuration=on|off" instead. Please say "... is deprecated, please use ...", to make it visible in "git-grep -i deprecat". > +"enforce-config-section" cannot be used together with "-global > +migration.send-configuration". If it happens, the behavior is > +undefined. Nasty. Could we catch and reject such invalid usage? > @end table > ETEXI
On Tue, Jul 04, 2017 at 10:06:54AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > It's never documented, and now we have one more parameter for it (which > > means this one can be obsolete in the future). Document it properly. > > > > Although now when enforce-config-section is set, it'll override the > > other "-global" parameter, that is not necessarily a rule. Forbid that > > usage in the document. > > > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > qemu-options.hx | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/qemu-options.hx b/qemu-options.hx > > index 297bd8a..927c51f 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off. > > @item s390-squash-mcss=on|off > > Enables or disables squashing subchannels into the default css. > > The default is off. > > +@item enforce-config-section=on|off > > +Decides whether we will send the configuration section when doing > > +migration. By default, it is turned on. We can set this to off to > > +explicitly disable it. Note: this parameter will be obsolete soon, > > +please use "-global migration.send-configuration=on|off" instead. > > Please say "... is deprecated, please use ...", to make it visible in > "git-grep -i deprecat". Both "obsolete" and "deprecated" are used in QEMU for such an usage, but I see indeed "deprecated" is used more often at least in qemu-options.hx (used six times) than "obsolete" (one time only). Let me switch then. :) > > > +"enforce-config-section" cannot be used together with "-global > > +migration.send-configuration". If it happens, the behavior is > > +undefined. > > Nasty. Could we catch and reject such invalid usage? Agree. However, looks like we cannot capture it easily on either of the parameters (or, can we?). Since we defined it as "undefined", it should be okay then, right? After all, iiuc enforce-config-section is already very rarely used (only on some special compat machines), and it'll be even more rare to use the new one without removing the old. Thanks,
(CCing Greg, the original author of the code that added the enforce-config-section option) On Tue, Jul 04, 2017 at 10:06:54AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > It's never documented, and now we have one more parameter for it (which > > means this one can be obsolete in the future). Document it properly. > > > > Although now when enforce-config-section is set, it'll override the > > other "-global" parameter, that is not necessarily a rule. Forbid that > > usage in the document. > > > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > qemu-options.hx | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/qemu-options.hx b/qemu-options.hx > > index 297bd8a..927c51f 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off. > > @item s390-squash-mcss=on|off > > Enables or disables squashing subchannels into the default css. > > The default is off. > > +@item enforce-config-section=on|off > > +Decides whether we will send the configuration section when doing > > +migration. By default, it is turned on. We can set this to off to > > +explicitly disable it. Note: this parameter will be obsolete soon, > > +please use "-global migration.send-configuration=on|off" instead. > > Please say "... is deprecated, please use ...", to make it visible in > "git-grep -i deprecat". > > > +"enforce-config-section" cannot be used together with "-global > > +migration.send-configuration". If it happens, the behavior is > > +undefined. > > Nasty. Could we catch and reject such invalid usage? Actually, the machine option will override migration.send-configuration=off, and I don't believe we will break that rule. Documenting that behavior (but warning that it is deprecated) sounds easier than adding extra code to detect the conflicting options. We can simply replace "is undefined" with "enforce-config-section will override migration.send-configuration, but enforce-config-section is deprecated". I don't think anybody is relying on that option. If nobody is using it, we can remove its code soon if we make it trigger a warning. Machine-type compatibility code is already using migration.send-configuration instead.
Eduardo Habkost <ehabkost@redhat.com> writes: > (CCing Greg, the original author of the code that added the > enforce-config-section option) > > On Tue, Jul 04, 2017 at 10:06:54AM +0200, Markus Armbruster wrote: >> Peter Xu <peterx@redhat.com> writes: >> >> > It's never documented, and now we have one more parameter for it (which >> > means this one can be obsolete in the future). Document it properly. >> > >> > Although now when enforce-config-section is set, it'll override the >> > other "-global" parameter, that is not necessarily a rule. Forbid that >> > usage in the document. >> > >> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> >> > Signed-off-by: Peter Xu <peterx@redhat.com> >> > --- >> > qemu-options.hx | 8 ++++++++ >> > 1 file changed, 8 insertions(+) >> > >> > diff --git a/qemu-options.hx b/qemu-options.hx >> > index 297bd8a..927c51f 100644 >> > --- a/qemu-options.hx >> > +++ b/qemu-options.hx >> > @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off. >> > @item s390-squash-mcss=on|off >> > Enables or disables squashing subchannels into the default css. >> > The default is off. >> > +@item enforce-config-section=on|off >> > +Decides whether we will send the configuration section when doing >> > +migration. By default, it is turned on. We can set this to off to >> > +explicitly disable it. Note: this parameter will be obsolete soon, >> > +please use "-global migration.send-configuration=on|off" instead. >> >> Please say "... is deprecated, please use ...", to make it visible in >> "git-grep -i deprecat". >> >> > +"enforce-config-section" cannot be used together with "-global >> > +migration.send-configuration". If it happens, the behavior is >> > +undefined. >> >> Nasty. Could we catch and reject such invalid usage? > > Actually, the machine option will override > migration.send-configuration=off, and I don't believe we will break that > rule. Documenting that behavior (but warning that it is deprecated) > sounds easier than adding extra code to detect the conflicting options. > > We can simply replace "is undefined" with "enforce-config-section will > override migration.send-configuration, but enforce-config-section is > deprecated". Better than the scary "behavior is undefined". But do we have to say anything at all? If somebody gives both options with different values, the conflicting settings fight it out. In a sane command line, the last one wins. Ours isn't sane. Do we really have to specify who wins? > I don't think anybody is relying on that option. If nobody is using it, > we can remove its code soon if we make it trigger a warning. > Machine-type compatibility code is already using > migration.send-configuration instead. What about @item enforce-config-section=on|off Controls sending of the configuration section when doing migration (default on). Note: this parameter is deprecated, please use "-global migration.send-configuration=on|off" instead.
On Wed, Jul 05, 2017 at 05:31:22PM +0200, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > (CCing Greg, the original author of the code that added the > > enforce-config-section option) > > > > On Tue, Jul 04, 2017 at 10:06:54AM +0200, Markus Armbruster wrote: > >> Peter Xu <peterx@redhat.com> writes: > >> > >> > It's never documented, and now we have one more parameter for it (which > >> > means this one can be obsolete in the future). Document it properly. > >> > > >> > Although now when enforce-config-section is set, it'll override the > >> > other "-global" parameter, that is not necessarily a rule. Forbid that > >> > usage in the document. > >> > > >> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > >> > Signed-off-by: Peter Xu <peterx@redhat.com> > >> > --- > >> > qemu-options.hx | 8 ++++++++ > >> > 1 file changed, 8 insertions(+) > >> > > >> > diff --git a/qemu-options.hx b/qemu-options.hx > >> > index 297bd8a..927c51f 100644 > >> > --- a/qemu-options.hx > >> > +++ b/qemu-options.hx > >> > @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off. > >> > @item s390-squash-mcss=on|off > >> > Enables or disables squashing subchannels into the default css. > >> > The default is off. > >> > +@item enforce-config-section=on|off > >> > +Decides whether we will send the configuration section when doing > >> > +migration. By default, it is turned on. We can set this to off to > >> > +explicitly disable it. Note: this parameter will be obsolete soon, > >> > +please use "-global migration.send-configuration=on|off" instead. > >> > >> Please say "... is deprecated, please use ...", to make it visible in > >> "git-grep -i deprecat". > >> > >> > +"enforce-config-section" cannot be used together with "-global > >> > +migration.send-configuration". If it happens, the behavior is > >> > +undefined. > >> > >> Nasty. Could we catch and reject such invalid usage? > > > > Actually, the machine option will override > > migration.send-configuration=off, and I don't believe we will break that > > rule. Documenting that behavior (but warning that it is deprecated) > > sounds easier than adding extra code to detect the conflicting options. > > > > We can simply replace "is undefined" with "enforce-config-section will > > override migration.send-configuration, but enforce-config-section is > > deprecated". > > Better than the scary "behavior is undefined". But do we have to say > anything at all? If somebody gives both options with different values, > the conflicting settings fight it out. In a sane command line, the last > one wins. Ours isn't sane. Do we really have to specify who wins? > > > I don't think anybody is relying on that option. If nobody is using it, > > we can remove its code soon if we make it trigger a warning. > > Machine-type compatibility code is already using > > migration.send-configuration instead. > > What about > > @item enforce-config-section=on|off > Controls sending of the configuration section when doing migration > (default on). Sorry to be misleading in current patch. Its default should be "off" but not "on" (as corrected by Eduardo, the point is we are talking about "enforce-config-section", not "migration.send-configuration"). Actually it won't make much sense if someone specify "off" here. > Note: this parameter is deprecated, please use > "-global migration.send-configuration=on|off" instead. This is a suggestion I would like to take. I'll remove the whole "undefined" sentence and update my post. Thanks,
diff --git a/qemu-options.hx b/qemu-options.hx index 297bd8a..927c51f 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off. @item s390-squash-mcss=on|off Enables or disables squashing subchannels into the default css. The default is off. +@item enforce-config-section=on|off +Decides whether we will send the configuration section when doing +migration. By default, it is turned on. We can set this to off to +explicitly disable it. Note: this parameter will be obsolete soon, +please use "-global migration.send-configuration=on|off" instead. +"enforce-config-section" cannot be used together with "-global +migration.send-configuration". If it happens, the behavior is +undefined. @end table ETEXI
It's never documented, and now we have one more parameter for it (which means this one can be obsolete in the future). Document it properly. Although now when enforce-config-section is set, it'll override the other "-global" parameter, that is not necessarily a rule. Forbid that usage in the document. Suggested-by: Eduardo Habkost <ehabkost@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- qemu-options.hx | 8 ++++++++ 1 file changed, 8 insertions(+)