The functionality of -writeconfig is limited and the code
does not even try to detect cases where it prints incorrect
syntax (for example if values have a quote in them, since
qemu_config_parse does not support any kind of escaping)
so remove it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
docs/system/deprecated.rst | 7 +++++++
qemu-options.hx | 7 +------
softmmu/vl.c | 1 +
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 2fcac7861e..561c916da2 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -146,6 +146,13 @@ library enabled as a cryptography provider.
Neither the ``nettle`` library, or the built-in cryptography provider are
supported on FIPS enabled hosts.
+``-writeconfig`` (since 6.0)
+'''''''''''''''''''''''''''''
+
+The ``-writeconfig`` option is not able to serialize the entire contents
+of the QEMU command line. It is thus considered a failed experiment
+and deprecated, with no current replacement.
+
QEMU Machine Protocol (QMP) commands
------------------------------------
diff --git a/qemu-options.hx b/qemu-options.hx
index 34be5a7a2d..252db9357c 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4335,13 +4335,8 @@ SRST
ERST
DEF("writeconfig", HAS_ARG, QEMU_OPTION_writeconfig,
"-writeconfig <file>\n"
- " read/write config file\n", QEMU_ARCH_ALL)
+ " read/write config file (deprecated)\n", QEMU_ARCH_ALL)
SRST
-``-writeconfig file``
- Write device configuration to file. The file can be either filename
- to save command line and device configuration into file or dash
- ``-``) character to print the output to stdout. This can be later
- used as input file for ``-readconfig`` option.
ERST
DEF("no-user-config", 0, QEMU_OPTION_nouserconfig,
diff --git a/softmmu/vl.c b/softmmu/vl.c
index b219ce1f35..6d8393b6f7 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -3356,6 +3356,7 @@ void qemu_init(int argc, char **argv, char **envp)
case QEMU_OPTION_writeconfig:
{
FILE *fp;
+ warn_report("-writeconfig is deprecated. It will go away in QEMU 6.2 with no replacement");
if (strcmp(optarg, "-") == 0) {
fp = stdout;
} else {
--
2.29.2
Paolo Bonzini <pbonzini@redhat.com> writes:
> The functionality of -writeconfig is limited and the code
> does not even try to detect cases where it prints incorrect
> syntax (for example if values have a quote in them, since
> qemu_config_parse does not support any kind of escaping)
> so remove it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> docs/system/deprecated.rst | 7 +++++++
> qemu-options.hx | 7 +------
> softmmu/vl.c | 1 +
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 2fcac7861e..561c916da2 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -146,6 +146,13 @@ library enabled as a cryptography provider.
> Neither the ``nettle`` library, or the built-in cryptography provider are
> supported on FIPS enabled hosts.
>
> +``-writeconfig`` (since 6.0)
> +'''''''''''''''''''''''''''''
> +
> +The ``-writeconfig`` option is not able to serialize the entire contents
> +of the QEMU command line. It is thus considered a failed experiment
> +and deprecated, with no current replacement.
> +
> QEMU Machine Protocol (QMP) commands
> ------------------------------------
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 34be5a7a2d..252db9357c 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4335,13 +4335,8 @@ SRST
> ERST
> DEF("writeconfig", HAS_ARG, QEMU_OPTION_writeconfig,
> "-writeconfig <file>\n"
> - " read/write config file\n", QEMU_ARCH_ALL)
> + " read/write config file (deprecated)\n", QEMU_ARCH_ALL)
> SRST
> -``-writeconfig file``
> - Write device configuration to file. The file can be either filename
> - to save command line and device configuration into file or dash
> - ``-``) character to print the output to stdout. This can be later
> - used as input file for ``-readconfig`` option.
> ERST
>
> DEF("no-user-config", 0, QEMU_OPTION_nouserconfig,
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index b219ce1f35..6d8393b6f7 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -3356,6 +3356,7 @@ void qemu_init(int argc, char **argv, char **envp)
> case QEMU_OPTION_writeconfig:
> {
> FILE *fp;
> + warn_report("-writeconfig is deprecated. It will go away in QEMU 6.2 with no replacement");
> if (strcmp(optarg, "-") == 0) {
> fp = stdout;
> } else {
Forgot to tweak the warning to "-writeconfig is deprecated and will go
away without a replacement"?
On 01/03/21 09:00, Markus Armbruster wrote:
>> + warn_report("-writeconfig is deprecated. It will go away in QEMU 6.2 with no replacement");
>> if (strcmp(optarg, "-") == 0) {
>> fp = stdout;
>> } else {
>
> Forgot to tweak the warning to "-writeconfig is deprecated and will go
> away without a replacement"?
Didn't really forget; being pretty sure that there's no usage in the
wild and having good reasons to remove the code, giving a firm removal
date should encourage people to speak up sooner rather than later.
Paolo
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 01/03/21 09:00, Markus Armbruster wrote:
>>> + warn_report("-writeconfig is deprecated. It will go away in QEMU 6.2 with no replacement");
>>> if (strcmp(optarg, "-") == 0) {
>>> fp = stdout;
>>> } else {
>>
>> Forgot to tweak the warning to "-writeconfig is deprecated and will go
>> away without a replacement"?
>
> Didn't really forget; being pretty sure that there's no usage in the
> wild and having good reasons to remove the code, giving a firm removal
> date should encourage people to speak up sooner rather than later.
Second thoughts after agreeing to change something are okay. Keeping
them for yourself not so much, because it deprives your reviewers of a
chance to raise further points.
In this case, the point I didn't make because I wanted to reach
agreement on contents before nitpicking form: you're not using
warn_report() the way it wants to be used:
/*
* Print a warning message to current monitor if we have one, else to stderr.
* Format arguments like sprintf(). The resulting message should be a
---> * single phrase, with no newline or trailing punctuation.
* Prepend the current location and append a newline.
*/
void warn_report(const char *fmt, ...)
Please tidy up.
On 01/03/21 14:26, Markus Armbruster wrote:
>> Didn't really forget; being pretty sure that there's no usage in the
>> wild and having good reasons to remove the code, giving a firm removal
>> date should encourage people to speak up sooner rather than later.
> Second thoughts after agreeing to change something are okay. Keeping
> them for yourself not so much, because it deprives your reviewers of a
> chance to raise further points.
Sorry about that.
> In this case, the point I didn't make because I wanted to reach
> agreement on contents before nitpicking form: you're not using
> warn_report() the way it wants to be used:
>
> /*
> * Print a warning message to current monitor if we have one, else to stderr.
> * Format arguments like sprintf(). The resulting message should be a
> ---> * single phrase, with no newline or trailing punctuation.
> * Prepend the current location and append a newline.
> */
> void warn_report(const char *fmt, ...)
I knew about the rules for no newline or trailing punctuation, but I
didn't remember the other. I can certainly respin, that said:
- the comment should say "sentence", not "phrase". For example "a
single phrase" is a single phrase, while "the resulting message should
be a single phrase" is a single sentence.
- I'm not sure how to interpret the rule above. First of all, the
sentence mixes part that are mandatory part ("no newline", checked by
checkpatch.pl) is mixed with the style guide ("no trailing punctuation"
and "a single sentence"). Second, whether a single sentence is better
often depends on the case. For example, comparing these four:
WARNING: -writeconfig foo: -writeconfig is deprecated. It will go away
in QEMU 6.2 with no replacement
WARNING: -writeconfig foo: -writeconfig is deprecated and will go away
in QEMU 6.2 with no replacement
WARNING: -writeconfig foo: -writeconfig is deprecated; it will go away
in QEMU 6.2 with no replacement
WARNING: -writeconfig foo: -writeconfig is deprecated
WARNING: -writeconfig foo: it will go away in QEMU 6.2 with no replacement
The first one is what was in this patch.
The second does sound fine to me and it's probably what I'll use in v2,
with or without the "in QEMU 6.2" part. However some could consider it
to be worse style due to the longer sentence.
The third one is playing with the rules; a semicolon would be separating
two sentences. However usage of the semicolon is quite common in error
messages, so maybe it would be good too?
The last one also complies, but it is not clear what "it" refers to so
it seems to be the worst in this case. In other cases it's the obvious
choice, and we even have an API for it (error_append_hint... however it
doesn't play well with error_fatal which I'm otherwise a big fan of).
In this case, not so much.
Paolo
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 01/03/21 14:26, Markus Armbruster wrote:
>>> Didn't really forget; being pretty sure that there's no usage in the
>>> wild and having good reasons to remove the code, giving a firm removal
>>> date should encourage people to speak up sooner rather than later.
>> Second thoughts after agreeing to change something are okay. Keeping
>> them for yourself not so much, because it deprives your reviewers of a
>> chance to raise further points.
>
> Sorry about that.
>
>> In this case, the point I didn't make because I wanted to reach
>> agreement on contents before nitpicking form: you're not using
>> warn_report() the way it wants to be used:
>>
>> /*
>> * Print a warning message to current monitor if we have one, else to stderr.
>> * Format arguments like sprintf(). The resulting message should be a
>> ---> * single phrase, with no newline or trailing punctuation.
>> * Prepend the current location and append a newline.
>> */
>> void warn_report(const char *fmt, ...)
>
> I knew about the rules for no newline or trailing punctuation, but I
> didn't remember the other. I can certainly respin, that said:
>
> - the comment should say "sentence", not "phrase". For example "a
> single phrase" is a single phrase, while "the resulting message should
> be a single phrase" is a single sentence.
I avoided "sentence", because good error messages aren't always
grammatically complete sentences. My use of "phrase" may well be wrong.
I tried! Patches welcome :)
Dicking around on the web, I just found
https://www.postgres-xl.org/documentation/error-style-guide.html
Drop the Postgres-specific parts, and what's left is pretty close to my
thoughts on error message style.
> - I'm not sure how to interpret the rule above. First of all, the
> sentence mixes part that are mandatory part ("no newline", checked by
> checkpatch.pl) is mixed with the style guide ("no trailing punctuation"
> and "a single sentence"). Second, whether a single sentence is better
> often depends on the case. For example, comparing these four:
>
> WARNING: -writeconfig foo: -writeconfig is deprecated. It will go away
> in QEMU 6.2 with no replacement
>
> WARNING: -writeconfig foo: -writeconfig is deprecated and will go away
> in QEMU 6.2 with no replacement
>
> WARNING: -writeconfig foo: -writeconfig is deprecated; it will go away
> in QEMU 6.2 with no replacement
>
> WARNING: -writeconfig foo: -writeconfig is deprecated
> WARNING: -writeconfig foo: it will go away in QEMU 6.2 with no replacement
>
> The first one is what was in this patch.
>
> The second does sound fine to me and it's probably what I'll use in v2,
> with or without the "in QEMU 6.2" part. However some could consider it
> to be worse style due to the longer sentence.
>
> The third one is playing with the rules; a semicolon would be separating
> two sentences. However usage of the semicolon is quite common in error
> messages, so maybe it would be good too?
Semicolons can be okay, as long the resulting message is still short.
Still short:
warning: -writeconfig foo: -writeconfig is deprecated without replacement
warning: -writeconfig foo: option is deprecated; there is no replacement
No longer short:
warning: -writeconfig foo: -writeconfig is deprecated; it will go away in QEMU 6.2 with no replacement
There is no need to squeeze all the information into the "primary" error
message! That one should state what's wrong *concisely*. If you feel
you should explain further, or would like to advise on what could be
done to fix the problem, a separate "hint" message often reads better
than overloading the primary message with information.
When reporting to the user, use error_report() / warn_report() for the
"primary", and error_printf() for the "hint".
When setting an Error, use error_setg() for the "primary", and
error_append_hint() for the "hint". error_report_err() will then use
error_report() and error_printf() correctly.
> The last one also complies, but it is not clear what "it" refers to so
> it seems to be the worst in this case. In other cases it's the obvious
> choice, and we even have an API for it (error_append_hint... however it
> doesn't play well with error_fatal which I'm otherwise a big fan of).
> In this case, not so much.
Use of error_append_hint() requires ERRP_GUARD(). Without ERRP_GUARD(),
the hint indeed gets lost when errp == &error_fatal.
Properly guarded, we could have something like
warning: -writeconfig foo: option -writeconfig is deprecated
This option will go away with no replacement.
I'm glad you like &error_fatal, too! I have had to defend it a few
times :)
On 01/03/21 15:54, Markus Armbruster wrote: > warning: -writeconfig foo: -writeconfig is deprecated without replacement > > warning: -writeconfig foo: option is deprecated; there is no replacement Hmm I don't know. I like the brevity, but I find it to be less user-friendly. And the "it" and "this option" sound wrong on a separate line. Maybe it's just me. Paolo > > Properly guarded, we could have something like > > warning: -writeconfig foo: option -writeconfig is deprecated > This option will go away with no replacement. > > I'm glad you like &error_fatal, too! I have had to defend it a few > times
On Fri, Feb 26, 2021 at 09:05:24AM +0100, Paolo Bonzini wrote:
> The functionality of -writeconfig is limited and the code
> does not even try to detect cases where it prints incorrect
> syntax (for example if values have a quote in them, since
> qemu_config_parse does not support any kind of escaping)
> so remove it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> docs/system/deprecated.rst | 7 +++++++
> qemu-options.hx | 7 +------
> softmmu/vl.c | 1 +
> 3 files changed, 9 insertions(+), 6 deletions(-)
[...]
Hi,
Sorry, I'm coming very late[1] to the discussion. Will there be a
replacement for '-readconfig'?
I agree with Gerd's comment[2] in the last year's thread (I missed to
notice at that time) about '-readconfig' being useful. I'm familiar
with least one hosting provider who uses[3] '-readconfig'. And I've
also used it for small snippets myself. I understand, these simple
use-cases doesn't make it right to keep it. :-)
I'm not saying "don't deprecate '-readconfig'", but just noting its
usefulness, even in its current form. So I'm just curious if there's
be a suggested replacement. Even if it means: "use libvirt; or use your
own bespoke scripts".
[1] https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg03681.html
"proposal: deprecate -readconfig/-writeconfig"
[2] https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg03681.html
[3] https://github.com/flyingcircusio/fc.qemu/blob/f789e57f605969a0/src/fc/qemu/agent.py#L1153
--
/kashyap
On 01/03/21 17:03, Kashyap Chamarthy wrote: > On Fri, Feb 26, 2021 at 09:05:24AM +0100, Paolo Bonzini wrote: >> The functionality of -writeconfig is limited and the code >> does not even try to detect cases where it prints incorrect >> syntax (for example if values have a quote in them, since >> qemu_config_parse does not support any kind of escaping) >> so remove it. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> docs/system/deprecated.rst | 7 +++++++ >> qemu-options.hx | 7 +------ >> softmmu/vl.c | 1 + >> 3 files changed, 9 insertions(+), 6 deletions(-) > > [...] > > Hi, > > Sorry, I'm coming very late[1] to the discussion. Will there be a > replacement for '-readconfig'? -readconfig is not being deprecated, there will be some code new to integrate it with the changes I'm planning to option parsing. Paolo > I agree with Gerd's comment[2] in the last year's thread (I missed to > notice at that time) about '-readconfig' being useful. I'm familiar > with least one hosting provider who uses[3] '-readconfig'. And I've > also used it for small snippets myself. I understand, these simple > use-cases doesn't make it right to keep it. :-) > > I'm not saying "don't deprecate '-readconfig'", but just noting its > usefulness, even in its current form. So I'm just curious if there's > be a suggested replacement. Even if it means: "use libvirt; or use your > own bespoke scripts". > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg03681.html > "proposal: deprecate -readconfig/-writeconfig" > [2] https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg03681.html > [3] https://github.com/flyingcircusio/fc.qemu/blob/f789e57f605969a0/src/fc/qemu/agent.py#L1153 >
On Mon, Mar 01, 2021 at 05:24:10PM +0100, Paolo Bonzini wrote: > On 01/03/21 17:03, Kashyap Chamarthy wrote: > > On Fri, Feb 26, 2021 at 09:05:24AM +0100, Paolo Bonzini wrote: > > > The functionality of -writeconfig is limited and the code > > > does not even try to detect cases where it prints incorrect > > > syntax (for example if values have a quote in them, since > > > qemu_config_parse does not support any kind of escaping) > > > so remove it. > > > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > > --- > > > docs/system/deprecated.rst | 7 +++++++ > > > qemu-options.hx | 7 +------ > > > softmmu/vl.c | 1 + > > > 3 files changed, 9 insertions(+), 6 deletions(-) > > > > [...] > > > > Hi, > > > > Sorry, I'm coming very late[1] to the discussion. Will there be a > > replacement for '-readconfig'? > > -readconfig is not being deprecated, there will be some code new to > integrate it with the changes I'm planning to option parsing. I see; thanks for the clarification. (I wasn't quite sure from the 2020 discussion thread.) [...] -- /kashyap
© 2016 - 2026 Red Hat, Inc.