All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.