[Qemu-devel] [PATCH v3 0/2] vl: Print warning if a non-empty default config file is found

[Qemu-devel] [PATCH v3 0/2] vl: Print warning if a non-empty default config file is found

[Eduardo Habkost]

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.12 or
2.13.

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: Print warning when a default config file is loaded

 block/blkdebug.c   |  1 -
 util/qemu-config.c | 15 +++++++--------
 vl.c               |  6 ++++++
 3 files changed, 13 insertions(+), 9 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH v3 1/2] config: qemu_config_parse() return number of config groups

[Eduardo Habkost]

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.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
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

Re: [Qemu-devel] [PATCH v3 1/2] config: qemu_config_parse() return number of config groups

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

On 09/27/2017 03:41 PM, 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.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

> 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>

Do you want the Suggested-by (spelled correctly) in the commit body proper?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v3 1/2] config: qemu_config_parse() return number of config groups

[Eduardo Habkost]

On Wed, Sep 27, 2017 at 03:49:55PM -0500, Eric Blake wrote:
> On 09/27/2017 03:41 PM, 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.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> > 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>
> 
> Do you want the Suggested-by (spelled correctly) in the commit body proper?

This line refers to a one-line change in the patch between v1 and
v2, only.  Doesn't seem appropriate for a Suggested-by line in
the commit message.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 1/2] config: qemu_config_parse() return number of config groups

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> 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.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v3 2/2] vl: Print warning when a default config file is loaded

[Eduardo Habkost]

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 add an additional message suggesting it as a
solution, but I thought it could make it more confusing. The
solution can be documented in the QEMU 2.11 ChangeLog.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v2 -> v3:
* Rebase (no code changes)
* Commit message update: suggest -no-user-config
---
 vl.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/vl.c b/vl.c
index 4fd01fda91..888649bbd1 100644
--- a/vl.c
+++ b/vl.c
@@ -3048,6 +3048,12 @@ static int qemu_read_default_config_file(void)
         return ret;
     }
 
+    if (ret > 0) {
+        loc_set_none();
+        error_report("Warning: Future QEMU versions won't load %s automatically",
+                     CONFIG_QEMU_CONFDIR "/qemu.conf");
+    }
+
     return 0;
 }
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH v3 2/2] vl: Print warning when a default config file is loaded

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

On 09/27/2017 03:41 PM, Eduardo Habkost wrote:
> 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 add an additional message suggesting it as a
> solution, but I thought it could make it more confusing. The
> solution can be documented in the QEMU 2.11 ChangeLog.
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v2 -> v3:
> * Rebase (no code changes)
> * Commit message update: suggest -no-user-config

> +    if (ret > 0) {
> +        loc_set_none();
> +        error_report("Warning: Future QEMU versions won't load %s automatically",
> +                     CONFIG_QEMU_CONFDIR "/qemu.conf");

Shouldn't we now use warn_report() with no 'Warning:' prefix?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v3 2/2] vl: Print warning when a default config file is loaded

[Eduardo Habkost]

On Wed, Sep 27, 2017 at 03:47:18PM -0500, Eric Blake wrote:
> On 09/27/2017 03:41 PM, Eduardo Habkost wrote:
> > 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 add an additional message suggesting it as a
> > solution, but I thought it could make it more confusing. The
> > solution can be documented in the QEMU 2.11 ChangeLog.
> > 
> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Changes v2 -> v3:
> > * Rebase (no code changes)
> > * Commit message update: suggest -no-user-config
> 
> > +    if (ret > 0) {
> > +        loc_set_none();
> > +        error_report("Warning: Future QEMU versions won't load %s automatically",
> > +                     CONFIG_QEMU_CONFDIR "/qemu.conf");
> 
> Shouldn't we now use warn_report() with no 'Warning:' prefix?

Oops, yes.  I will fix it and send v4.

-- 
Eduardo