* [Qemu-devel] [PATCH v4 0/2] vl: Deprecate auto-loading of qemu.conf
@ 2017-10-04 2:50 Eduardo Habkost
2017-10-04 2:50 ` [Qemu-devel] [PATCH v4 1/2] config: qemu_config_parse() return number of config groups Eduardo Habkost
2017-10-04 2:50 ` [Qemu-devel] [PATCH v4 2/2] vl: Deprecate auto-loading of qemu.conf Eduardo Habkost
0 siblings, 2 replies; 9+ messages in thread
From: Eduardo Habkost @ 2017-10-04 2:50 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, Paolo Bonzini, Eric Blake, Daniel P. Berrange
This missed v2.9 and v2.10. Let's try again: we can include this
on v2.11, and remove the default config file in QEMU 2.13 or
2.14.
Changes v3 -> v4:
* Use warn_report() instead of error_report("warning: ...")
(Eric Blake)
* Document as a deprecated feature in qemu-doc.texi
* Updated Subject line
(was "vl: Print warning if a non-empty default config file is found")
Changes v2 -> v3:
* Rebase to latest qemu.git master
Changes v1 -> v2:
* Remove unnecessary translation of qemu_config_parse()
erros to -EINVAL at block/blkdebug.c:read_config()
* Suggsted-by: Markus Armbruster <armbru@redhat.com>
We plan to remove support for /etc/qemu/qemu.conf in the near
future. Make QEMU print a warning in case there a non-empty
/etc/qemu/qemu.conf is loaded, so users have time to adapt.
Eduardo Habkost (2):
config: qemu_config_parse() return number of config groups
vl: Deprecate auto-loading of qemu.conf
block/blkdebug.c | 1 -
util/qemu-config.c | 15 +++++++--------
vl.c | 6 ++++++
qemu-doc.texi | 8 ++++++++
4 files changed, 21 insertions(+), 9 deletions(-)
--
2.13.5
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v4 1/2] config: qemu_config_parse() return number of config groups
2017-10-04 2:50 [Qemu-devel] [PATCH v4 0/2] vl: Deprecate auto-loading of qemu.conf Eduardo Habkost
@ 2017-10-04 2:50 ` Eduardo Habkost
2017-10-04 20:41 ` Eduardo Habkost
2017-10-04 2:50 ` [Qemu-devel] [PATCH v4 2/2] vl: Deprecate auto-loading of qemu.conf Eduardo Habkost
1 sibling, 1 reply; 9+ messages in thread
From: Eduardo Habkost @ 2017-10-04 2:50 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, Paolo Bonzini, Eric Blake, Daniel P. Berrange
Change qemu_config_parse() to return the number of config groups
in success and -EINVAL on error. This will allow callers of
qemu_config_parse() to check if something was really loaded from
the config file.
All existing callers of qemu_config_parse() and
qemu_read_config_file() only check if the return value was
negative, so the change shouldn't affect them.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes series v3 -> series v4:
* (none)
Changes v2 -> v3:
* None (rebase only)
Changes v1 -> v2:
* Remove unnecessary translation of qemu_config_parse()
erros to -EINVAL at block/blkdebug.c:read_config()
* Suggsted-by: Markus Armbruster <armbru@redhat.com>
---
block/blkdebug.c | 1 -
util/qemu-config.c | 15 +++++++--------
2 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 46e53f2f09..dfdf9b91aa 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -244,7 +244,6 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
ret = qemu_config_parse(f, config_groups, filename);
if (ret < 0) {
error_setg(errp, "Could not parse blkdebug config file");
- ret = -EINVAL;
goto fail;
}
}
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 405dd1a1d7..99b0e46fa3 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -385,6 +385,7 @@ void qemu_config_write(FILE *fp)
}
}
+/* Returns number of config groups on success, -errno on error */
int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
{
char line[1024], group[64], id[64], arg[64], value[1024];
@@ -392,7 +393,8 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
QemuOptsList *list = NULL;
Error *local_err = NULL;
QemuOpts *opts = NULL;
- int res = -1, lno = 0;
+ int res = -EINVAL, lno = 0;
+ int count = 0;
loc_push_none(&loc);
while (fgets(line, sizeof(line), fp) != NULL) {
@@ -413,6 +415,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
goto out;
}
opts = qemu_opts_create(list, id, 1, NULL);
+ count++;
continue;
}
if (sscanf(line, "[%63[^]]]", group) == 1) {
@@ -423,6 +426,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
goto out;
}
opts = qemu_opts_create(list, NULL, 0, &error_abort);
+ count++;
continue;
}
value[0] = '\0';
@@ -447,7 +451,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
error_report("error reading file");
goto out;
}
- res = 0;
+ res = count;
out:
loc_pop(&loc);
return res;
@@ -464,12 +468,7 @@ int qemu_read_config_file(const char *filename)
ret = qemu_config_parse(f, vm_config_groups, filename);
fclose(f);
-
- if (ret == 0) {
- return 0;
- } else {
- return -EINVAL;
- }
+ return ret;
}
static void config_parse_qdict_section(QDict *options, QemuOptsList *opts,
--
2.13.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v4 2/2] vl: Deprecate auto-loading of qemu.conf
2017-10-04 2:50 [Qemu-devel] [PATCH v4 0/2] vl: Deprecate auto-loading of qemu.conf Eduardo Habkost
2017-10-04 2:50 ` [Qemu-devel] [PATCH v4 1/2] config: qemu_config_parse() return number of config groups Eduardo Habkost
@ 2017-10-04 2:50 ` Eduardo Habkost
2017-10-04 5:42 ` Markus Armbruster
1 sibling, 1 reply; 9+ messages in thread
From: Eduardo Habkost @ 2017-10-04 2:50 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, Paolo Bonzini, Eric Blake, Daniel P. Berrange
In case there were options set in the default config file, print
a warning so users can update their scripts.
If somebody wants to keep the config file as-is, avoid the
warning and use a command-line that will work in future QEMU
versions, they can use:
$QEMU -no-user-config -readconfig /etc/qemu/qemu.conf
I was going to include the suggestion in the warning message, but
I thought it could make it more confusing. The suggestion is
documented in qemu-doc.texi.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v3 -> v4:
* Use warn_report() instead of error_report("warning: ...")
(Eric Blake)
* Document as a deprecated feature in qemu-doc.texi
* Update subject line
(was: "vl: Print warning when a default config file is loaded")
Changes v2 -> v3:
* Rebase (no code changes)
* Commit message update: suggest -no-user-config
---
vl.c | 6 ++++++
qemu-doc.texi | 8 ++++++++
2 files changed, 14 insertions(+)
diff --git a/vl.c b/vl.c
index 3fed457921..1b0ecdf74e 100644
--- a/vl.c
+++ b/vl.c
@@ -3066,6 +3066,12 @@ static int qemu_read_default_config_file(void)
return ret;
}
+ if (ret > 0) {
+ loc_set_none();
+ warn_report("Future QEMU versions won't load %s automatically",
+ CONFIG_QEMU_CONFDIR "/qemu.conf");
+ }
+
return 0;
}
diff --git a/qemu-doc.texi b/qemu-doc.texi
index ecd186a159..a81a09d05c 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2370,6 +2370,14 @@ they were first deprecated in the 2.10.0 release.
What follows is a list of all features currently marked as
deprecated.
+@section Automatic loading of @file{qemu.conf} (since 2.11.0)
+
+The automatic loading of an user-provided @file{qemu.conf} file from the QEMU
+config directory is deprecated and behavior will change in future QEMU versions.
+To load an user-provided @file{qemu.conf} file and keep compatibility with
+future versions, the arguments @samp{-no-user-config -readconfig
+@var{CONFDIR}/qemu.conf} may be used.
+
@section System emulator command line arguments
@subsection -drive boot=on|off (since 1.3.0)
--
2.13.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] vl: Deprecate auto-loading of qemu.conf
2017-10-04 2:50 ` [Qemu-devel] [PATCH v4 2/2] vl: Deprecate auto-loading of qemu.conf Eduardo Habkost
@ 2017-10-04 5:42 ` Markus Armbruster
2017-10-04 12:23 ` Eduardo Habkost
0 siblings, 1 reply; 9+ messages in thread
From: Markus Armbruster @ 2017-10-04 5:42 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: qemu-devel, Paolo Bonzini
Eduardo Habkost <ehabkost@redhat.com> writes:
> In case there were options set in the default config file, print
> a warning so users can update their scripts.
>
> If somebody wants to keep the config file as-is, avoid the
> warning and use a command-line that will work in future QEMU
> versions, they can use:
>
> $QEMU -no-user-config -readconfig /etc/qemu/qemu.conf
>
> I was going to include the suggestion in the warning message, but
> I thought it could make it more confusing. The suggestion is
> documented in qemu-doc.texi.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v3 -> v4:
> * Use warn_report() instead of error_report("warning: ...")
> (Eric Blake)
> * Document as a deprecated feature in qemu-doc.texi
> * Update subject line
> (was: "vl: Print warning when a default config file is loaded")
>
> Changes v2 -> v3:
> * Rebase (no code changes)
> * Commit message update: suggest -no-user-config
> ---
> vl.c | 6 ++++++
> qemu-doc.texi | 8 ++++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/vl.c b/vl.c
> index 3fed457921..1b0ecdf74e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3066,6 +3066,12 @@ static int qemu_read_default_config_file(void)
> return ret;
> }
>
> + if (ret > 0) {
> + loc_set_none();
Sure we need this here?
> + warn_report("Future QEMU versions won't load %s automatically",
> + CONFIG_QEMU_CONFDIR "/qemu.conf");
> + }
> +
> return 0;
> }
>
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index ecd186a159..a81a09d05c 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -2370,6 +2370,14 @@ they were first deprecated in the 2.10.0 release.
> What follows is a list of all features currently marked as
> deprecated.
>
> +@section Automatic loading of @file{qemu.conf} (since 2.11.0)
> +
> +The automatic loading of an user-provided @file{qemu.conf} file from the QEMU
> +config directory is deprecated and behavior will change in future QEMU versions.
> +To load an user-provided @file{qemu.conf} file and keep compatibility with
> +future versions, the arguments @samp{-no-user-config -readconfig
> +@var{CONFDIR}/qemu.conf} may be used.
> +
> @section System emulator command line arguments
>
> @subsection -drive boot=on|off (since 1.3.0)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] vl: Deprecate auto-loading of qemu.conf
2017-10-04 5:42 ` Markus Armbruster
@ 2017-10-04 12:23 ` Eduardo Habkost
2017-10-04 20:57 ` Eduardo Habkost
0 siblings, 1 reply; 9+ messages in thread
From: Eduardo Habkost @ 2017-10-04 12:23 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Paolo Bonzini
On Wed, Oct 04, 2017 at 07:42:17AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > In case there were options set in the default config file, print
> > a warning so users can update their scripts.
> >
> > If somebody wants to keep the config file as-is, avoid the
> > warning and use a command-line that will work in future QEMU
> > versions, they can use:
> >
> > $QEMU -no-user-config -readconfig /etc/qemu/qemu.conf
> >
> > I was going to include the suggestion in the warning message, but
> > I thought it could make it more confusing. The suggestion is
> > documented in qemu-doc.texi.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Changes v3 -> v4:
> > * Use warn_report() instead of error_report("warning: ...")
> > (Eric Blake)
> > * Document as a deprecated feature in qemu-doc.texi
> > * Update subject line
> > (was: "vl: Print warning when a default config file is loaded")
> >
> > Changes v2 -> v3:
> > * Rebase (no code changes)
> > * Commit message update: suggest -no-user-config
> > ---
> > vl.c | 6 ++++++
> > qemu-doc.texi | 8 ++++++++
> > 2 files changed, 14 insertions(+)
> >
> > diff --git a/vl.c b/vl.c
> > index 3fed457921..1b0ecdf74e 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3066,6 +3066,12 @@ static int qemu_read_default_config_file(void)
> > return ret;
> > }
> >
> > + if (ret > 0) {
> > + loc_set_none();
>
> Sure we need this here?
IIRC we needed it in the original version, but I don't remember
why. I will check this.
>
> > + warn_report("Future QEMU versions won't load %s automatically",
> > + CONFIG_QEMU_CONFDIR "/qemu.conf");
> > + }
> > +
> > return 0;
> > }
> >
> > diff --git a/qemu-doc.texi b/qemu-doc.texi
> > index ecd186a159..a81a09d05c 100644
> > --- a/qemu-doc.texi
> > +++ b/qemu-doc.texi
> > @@ -2370,6 +2370,14 @@ they were first deprecated in the 2.10.0 release.
> > What follows is a list of all features currently marked as
> > deprecated.
> >
> > +@section Automatic loading of @file{qemu.conf} (since 2.11.0)
> > +
> > +The automatic loading of an user-provided @file{qemu.conf} file from the QEMU
> > +config directory is deprecated and behavior will change in future QEMU versions.
> > +To load an user-provided @file{qemu.conf} file and keep compatibility with
> > +future versions, the arguments @samp{-no-user-config -readconfig
> > +@var{CONFDIR}/qemu.conf} may be used.
> > +
> > @section System emulator command line arguments
> >
> > @subsection -drive boot=on|off (since 1.3.0)
--
Eduardo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] config: qemu_config_parse() return number of config groups
2017-10-04 2:50 ` [Qemu-devel] [PATCH v4 1/2] config: qemu_config_parse() return number of config groups Eduardo Habkost
@ 2017-10-04 20:41 ` Eduardo Habkost
0 siblings, 0 replies; 9+ messages in thread
From: Eduardo Habkost @ 2017-10-04 20:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Markus Armbruster
On Tue, Oct 03, 2017 at 11:50:42PM -0300, Eduardo Habkost wrote:
> Change qemu_config_parse() to return the number of config groups
> in success and -EINVAL on error. This will allow callers of
> qemu_config_parse() to check if something was really loaded from
> the config file.
>
> All existing callers of qemu_config_parse() and
> qemu_read_config_file() only check if the return value was
> negative, so the change shouldn't affect them.
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Paolo, if you are OK with this I'm queueing it (only 1/2 by now)
on machine-next.
--
Eduardo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] vl: Deprecate auto-loading of qemu.conf
2017-10-04 12:23 ` Eduardo Habkost
@ 2017-10-04 20:57 ` Eduardo Habkost
2017-10-05 5:00 ` Markus Armbruster
0 siblings, 1 reply; 9+ messages in thread
From: Eduardo Habkost @ 2017-10-04 20:57 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Paolo Bonzini, qemu-devel
On Wed, Oct 04, 2017 at 09:23:08AM -0300, Eduardo Habkost wrote:
> On Wed, Oct 04, 2017 at 07:42:17AM +0200, Markus Armbruster wrote:
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> >
> > > In case there were options set in the default config file, print
> > > a warning so users can update their scripts.
> > >
> > > If somebody wants to keep the config file as-is, avoid the
> > > warning and use a command-line that will work in future QEMU
> > > versions, they can use:
> > >
> > > $QEMU -no-user-config -readconfig /etc/qemu/qemu.conf
> > >
> > > I was going to include the suggestion in the warning message, but
> > > I thought it could make it more confusing. The suggestion is
> > > documented in qemu-doc.texi.
> > >
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > Changes v3 -> v4:
> > > * Use warn_report() instead of error_report("warning: ...")
> > > (Eric Blake)
> > > * Document as a deprecated feature in qemu-doc.texi
> > > * Update subject line
> > > (was: "vl: Print warning when a default config file is loaded")
> > >
> > > Changes v2 -> v3:
> > > * Rebase (no code changes)
> > > * Commit message update: suggest -no-user-config
> > > ---
> > > vl.c | 6 ++++++
> > > qemu-doc.texi | 8 ++++++++
> > > 2 files changed, 14 insertions(+)
> > >
> > > diff --git a/vl.c b/vl.c
> > > index 3fed457921..1b0ecdf74e 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -3066,6 +3066,12 @@ static int qemu_read_default_config_file(void)
> > > return ret;
> > > }
> > >
> > > + if (ret > 0) {
> > > + loc_set_none();
> >
> > Sure we need this here?
>
> IIRC we needed it in the original version, but I don't remember
> why. I will check this.
This is the result if we don't clear error location state:
$ ./x86_64-softmmu/qemu-system-x86_64 -monitor stdio -display none -device foobar
qemu-system-x86_64: -device foobar: warning: Future QEMU versions won't load /usr/local/etc/qemu/qemu.conf automatically
^^^^^^^^^^^^^^
However, it's probably better to do this right after the loop,
just like we already do in the second option parsing loop:
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
diff --git a/vl.c b/vl.c
index f9acc17c01..a8fd247d71 100644
--- a/vl.c
+++ b/vl.c
@@ -3067,7 +3067,6 @@ static int qemu_read_default_config_file(void)
}
if (ret > 0) {
- loc_set_none();
warn_report("Future QEMU versions won't load %s automatically",
CONFIG_QEMU_CONFDIR "/qemu.conf");
}
@@ -3224,6 +3223,11 @@ int main(int argc, char **argv, char **envp)
}
}
}
+ /*
+ * Clear error location left behind by the loop.
+ * Best done right after the loop. Do not insert code here!
+ */
+ loc_set_none();
if (userconfig) {
if (qemu_read_default_config_file() < 0) {
--
Eduardo
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] vl: Deprecate auto-loading of qemu.conf
2017-10-04 20:57 ` Eduardo Habkost
@ 2017-10-05 5:00 ` Markus Armbruster
2017-10-05 12:34 ` Eduardo Habkost
0 siblings, 1 reply; 9+ messages in thread
From: Markus Armbruster @ 2017-10-05 5:00 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, qemu-devel
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Wed, Oct 04, 2017 at 09:23:08AM -0300, Eduardo Habkost wrote:
>> On Wed, Oct 04, 2017 at 07:42:17AM +0200, Markus Armbruster wrote:
>> > Eduardo Habkost <ehabkost@redhat.com> writes:
>> >
>> > > In case there were options set in the default config file, print
>> > > a warning so users can update their scripts.
>> > >
>> > > If somebody wants to keep the config file as-is, avoid the
>> > > warning and use a command-line that will work in future QEMU
>> > > versions, they can use:
>> > >
>> > > $QEMU -no-user-config -readconfig /etc/qemu/qemu.conf
>> > >
>> > > I was going to include the suggestion in the warning message, but
>> > > I thought it could make it more confusing. The suggestion is
>> > > documented in qemu-doc.texi.
>> > >
>> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> > > ---
>> > > Changes v3 -> v4:
>> > > * Use warn_report() instead of error_report("warning: ...")
>> > > (Eric Blake)
>> > > * Document as a deprecated feature in qemu-doc.texi
>> > > * Update subject line
>> > > (was: "vl: Print warning when a default config file is loaded")
>> > >
>> > > Changes v2 -> v3:
>> > > * Rebase (no code changes)
>> > > * Commit message update: suggest -no-user-config
>> > > ---
>> > > vl.c | 6 ++++++
>> > > qemu-doc.texi | 8 ++++++++
>> > > 2 files changed, 14 insertions(+)
>> > >
>> > > diff --git a/vl.c b/vl.c
>> > > index 3fed457921..1b0ecdf74e 100644
>> > > --- a/vl.c
>> > > +++ b/vl.c
>> > > @@ -3066,6 +3066,12 @@ static int qemu_read_default_config_file(void)
>> > > return ret;
>> > > }
>> > >
>> > > + if (ret > 0) {
>> > > + loc_set_none();
>> >
>> > Sure we need this here?
>>
>> IIRC we needed it in the original version, but I don't remember
>> why. I will check this.
>
> This is the result if we don't clear error location state:
>
> $ ./x86_64-softmmu/qemu-system-x86_64 -monitor stdio -display none -device foobar
> qemu-system-x86_64: -device foobar: warning: Future QEMU versions won't load /usr/local/etc/qemu/qemu.conf automatically
> ^^^^^^^^^^^^^^
>
> However, it's probably better to do this right after the loop,
> just like we already do in the second option parsing loop:
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> diff --git a/vl.c b/vl.c
> index f9acc17c01..a8fd247d71 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3067,7 +3067,6 @@ static int qemu_read_default_config_file(void)
> }
>
> if (ret > 0) {
> - loc_set_none();
> warn_report("Future QEMU versions won't load %s automatically",
> CONFIG_QEMU_CONFDIR "/qemu.conf");
> }
> @@ -3224,6 +3223,11 @@ int main(int argc, char **argv, char **envp)
> }
> }
> }
> + /*
> + * Clear error location left behind by the loop.
> + * Best done right after the loop. Do not insert code here!
> + */
> + loc_set_none();
>
> if (userconfig) {
> if (qemu_read_default_config_file() < 0) {
Hmm, could this hunk be a bug fix? Consider before the patch:
qemu_read_default_config_file()
qemu_read_config_file()
qemu_config_parse()
error_report() or error_report_err()
I suspect this uses current location left behind by the loop
inappropriately. If true, the hunk should be a separate bug fix patch.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] vl: Deprecate auto-loading of qemu.conf
2017-10-05 5:00 ` Markus Armbruster
@ 2017-10-05 12:34 ` Eduardo Habkost
0 siblings, 0 replies; 9+ messages in thread
From: Eduardo Habkost @ 2017-10-05 12:34 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Paolo Bonzini, qemu-devel
On Thu, Oct 05, 2017 at 07:00:33AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > On Wed, Oct 04, 2017 at 09:23:08AM -0300, Eduardo Habkost wrote:
> >> On Wed, Oct 04, 2017 at 07:42:17AM +0200, Markus Armbruster wrote:
> >> > Eduardo Habkost <ehabkost@redhat.com> writes:
> >> >
> >> > > In case there were options set in the default config file, print
> >> > > a warning so users can update their scripts.
> >> > >
> >> > > If somebody wants to keep the config file as-is, avoid the
> >> > > warning and use a command-line that will work in future QEMU
> >> > > versions, they can use:
> >> > >
> >> > > $QEMU -no-user-config -readconfig /etc/qemu/qemu.conf
> >> > >
> >> > > I was going to include the suggestion in the warning message, but
> >> > > I thought it could make it more confusing. The suggestion is
> >> > > documented in qemu-doc.texi.
> >> > >
> >> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> > > ---
> >> > > Changes v3 -> v4:
> >> > > * Use warn_report() instead of error_report("warning: ...")
> >> > > (Eric Blake)
> >> > > * Document as a deprecated feature in qemu-doc.texi
> >> > > * Update subject line
> >> > > (was: "vl: Print warning when a default config file is loaded")
> >> > >
> >> > > Changes v2 -> v3:
> >> > > * Rebase (no code changes)
> >> > > * Commit message update: suggest -no-user-config
> >> > > ---
> >> > > vl.c | 6 ++++++
> >> > > qemu-doc.texi | 8 ++++++++
> >> > > 2 files changed, 14 insertions(+)
> >> > >
> >> > > diff --git a/vl.c b/vl.c
> >> > > index 3fed457921..1b0ecdf74e 100644
> >> > > --- a/vl.c
> >> > > +++ b/vl.c
> >> > > @@ -3066,6 +3066,12 @@ static int qemu_read_default_config_file(void)
> >> > > return ret;
> >> > > }
> >> > >
> >> > > + if (ret > 0) {
> >> > > + loc_set_none();
> >> >
> >> > Sure we need this here?
> >>
> >> IIRC we needed it in the original version, but I don't remember
> >> why. I will check this.
> >
> > This is the result if we don't clear error location state:
> >
> > $ ./x86_64-softmmu/qemu-system-x86_64 -monitor stdio -display none -device foobar
> > qemu-system-x86_64: -device foobar: warning: Future QEMU versions won't load /usr/local/etc/qemu/qemu.conf automatically
> > ^^^^^^^^^^^^^^
> >
> > However, it's probably better to do this right after the loop,
> > just like we already do in the second option parsing loop:
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > diff --git a/vl.c b/vl.c
> > index f9acc17c01..a8fd247d71 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3067,7 +3067,6 @@ static int qemu_read_default_config_file(void)
> > }
> >
> > if (ret > 0) {
> > - loc_set_none();
> > warn_report("Future QEMU versions won't load %s automatically",
> > CONFIG_QEMU_CONFDIR "/qemu.conf");
> > }
> > @@ -3224,6 +3223,11 @@ int main(int argc, char **argv, char **envp)
> > }
> > }
> > }
> > + /*
> > + * Clear error location left behind by the loop.
> > + * Best done right after the loop. Do not insert code here!
> > + */
> > + loc_set_none();
> >
> > if (userconfig) {
> > if (qemu_read_default_config_file() < 0) {
>
> Hmm, could this hunk be a bug fix? Consider before the patch:
>
> qemu_read_default_config_file()
> qemu_read_config_file()
> qemu_config_parse()
> error_report() or error_report_err()
>
> I suspect this uses current location left behind by the loop
> inappropriately. If true, the hunk should be a separate bug fix patch.
It does, but nothing was broken because the only thing between
the two option parsing loops is qemu_read_default_config_file(),
and qemu_config_parse() calls loc_push_none() immediately.
But it makes sense as a separate patch anyway. I will send a new
series.
--
Eduardo
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-10-05 12:34 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-04 2:50 [Qemu-devel] [PATCH v4 0/2] vl: Deprecate auto-loading of qemu.conf Eduardo Habkost
2017-10-04 2:50 ` [Qemu-devel] [PATCH v4 1/2] config: qemu_config_parse() return number of config groups Eduardo Habkost
2017-10-04 20:41 ` Eduardo Habkost
2017-10-04 2:50 ` [Qemu-devel] [PATCH v4 2/2] vl: Deprecate auto-loading of qemu.conf Eduardo Habkost
2017-10-04 5:42 ` Markus Armbruster
2017-10-04 12:23 ` Eduardo Habkost
2017-10-04 20:57 ` Eduardo Habkost
2017-10-05 5:00 ` Markus Armbruster
2017-10-05 12:34 ` Eduardo Habkost
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.