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

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

[Eduardo Habkost]

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.

This series is based on my machine-next branch, at:
  https://github.com/ehabkost/qemu.git machine-next

Eduardo Habkost (2):
  config: qemu_config_parse() return number of config groups
  vl: Print warning when a default config file is loaded

 util/qemu-config.c | 15 +++++++--------
 vl.c               |  6 ++++++
 2 files changed, 13 insertions(+), 8 deletions(-)

-- 
2.11.0.259.g40922b1

[Qemu-devel] [PATCH 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>
---
 util/qemu-config.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/util/qemu-config.c b/util/qemu-config.c
index 5527100a01..560c201bd3 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -379,6 +379,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];
@@ -386,7 +387,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) {
@@ -407,6 +409,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) {
@@ -417,6 +420,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';
@@ -441,7 +445,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;
@@ -458,12 +462,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.11.0.259.g40922b1

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

[Markus Armbruster]

Uh, I totally forgot about this series.  My apologies...

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.

Two of them:

* read_config() maps negative value to -EINVAL.  Callers:

  - blkdebug_open() passes it on.  As a .bdrv_file_open() method, it's
    supposed to return -errno on failure.  Good.

* qemu_read_config_file() maps non-zero value to -EINVAL.  Callers:

  - qemu_read_default_config_file() maps -EINVAL to zero.  WTF?

  - main() passes sterror(EINVAL) to error_report().  Good.

Also: qemu_config_parse() reports errors with error_report().  Let's
have another look at its callers:

* read_config() has an Error ** parameter.  Bad.  Care to convert the
  sucker to Error?

* qemu_read_config_file() doesn't report errors.  Callers:

  - qemu_read_default_config_file() doesn't report errors.  Its called
    by main(), and ...

  - main() reports with error_report().  Good.

>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  util/qemu-config.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 5527100a01..560c201bd3 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -379,6 +379,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];
> @@ -386,7 +387,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) {
> @@ -407,6 +409,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) {
> @@ -417,6 +420,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';
> @@ -441,7 +445,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;
> @@ -458,12 +462,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,

I think this mapping to -EINVAL is also superfluous now:

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 6117ce5..fbefa9e 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -252,7 +252,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;
         }
     }

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

[Eduardo Habkost]

On Mon, Feb 27, 2017 at 06:59:51PM +0100, Markus Armbruster wrote:
> Uh, I totally forgot about this series.  My apologies...
> 
> 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.
> 
> Two of them:
> 
> * read_config() maps negative value to -EINVAL.  Callers:
> 
>   - blkdebug_open() passes it on.  As a .bdrv_file_open() method, it's
>     supposed to return -errno on failure.  Good.
> 
> * qemu_read_config_file() maps non-zero value to -EINVAL.  Callers:
> 
>   - qemu_read_default_config_file() maps -EINVAL to zero.  WTF?

qemu_read_config_file() maps -ENOENT to zero, not -EINVAL. It
will ignore the error only if the default config file doesn't
exist (if fopen() sets errno to ENOENT).

> 
>   - main() passes sterror(EINVAL) to error_report().  Good.
> 
> Also: qemu_config_parse() reports errors with error_report().  Let's
> have another look at its callers:
> 
> * read_config() has an Error ** parameter.  Bad.  Care to convert the
>   sucker to Error?

I can do it in a separate patch, unless you believe it must be
done by this patch patch in a single step.

> 
> * qemu_read_config_file() doesn't report errors.  Callers:
> 
>   - qemu_read_default_config_file() doesn't report errors.  Its called
>     by main(), and ...

This means fopen() errors won't get reported. Good catch. I will
send a separate fix.

> 
>   - main() reports with error_report().  Good.
> 
[...]
> 
> I think this mapping to -EINVAL is also superfluous now:
> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 6117ce5..fbefa9e 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -252,7 +252,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;

Good catch. I will squash it into the patch.

-- 
Eduardo

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Feb 27, 2017 at 06:59:51PM +0100, Markus Armbruster wrote:
>> Uh, I totally forgot about this series.  My apologies...
>> 
>> 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.
>> 
>> Two of them:
>> 
>> * read_config() maps negative value to -EINVAL.  Callers:
>> 
>>   - blkdebug_open() passes it on.  As a .bdrv_file_open() method, it's
>>     supposed to return -errno on failure.  Good.
>> 
>> * qemu_read_config_file() maps non-zero value to -EINVAL.  Callers:
>> 
>>   - qemu_read_default_config_file() maps -EINVAL to zero.  WTF?
>
> qemu_read_config_file() maps -ENOENT to zero, not -EINVAL. It
> will ignore the error only if the default config file doesn't
> exist (if fopen() sets errno to ENOENT).

Crosseyed reviewer syndrome...

>>   - main() passes sterror(EINVAL) to error_report().  Good.
>> 
>> Also: qemu_config_parse() reports errors with error_report().  Let's
>> have another look at its callers:
>> 
>> * read_config() has an Error ** parameter.  Bad.  Care to convert the
>>   sucker to Error?
>
> I can do it in a separate patch, unless you believe it must be
> done by this patch patch in a single step.

It *should* be a separate patch.

>> 
>> * qemu_read_config_file() doesn't report errors.  Callers:
>> 
>>   - qemu_read_default_config_file() doesn't report errors.  Its called
>>     by main(), and ...
>
> This means fopen() errors won't get reported. Good catch. I will
> send a separate fix.
>
>> 
>>   - main() reports with error_report().  Good.
>> 
> [...]
>> 
>> I think this mapping to -EINVAL is also superfluous now:
>> 
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 6117ce5..fbefa9e 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -252,7 +252,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;
>
> Good catch. I will squash it into the patch.

Looking forward to v2 :)

[Qemu-devel] [PATCH 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 -nodefconfig -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.9 ChangeLog.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 vl.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/vl.c b/vl.c
index b563f9b924..e80b6da4bd 100644
--- a/vl.c
+++ b/vl.c
@@ -2999,6 +2999,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.11.0.259.g40922b1

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

[Markus Armbruster]

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.

Can you explain why you don't warn on an empty qemu.conf?

> 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 -nodefconfig -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.9 ChangeLog.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  vl.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/vl.c b/vl.c
> index b563f9b924..e80b6da4bd 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2999,6 +2999,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;
>  }

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

[Eduardo Habkost]

On Wed, Jan 18, 2017 at 05:04:45PM +0100, 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.
> 
> Can you explain why you don't warn on an empty qemu.conf?

I didnt't want to show the warning to users that won't be
affected by the change. I assume that users with empty qemu.conf
files won't care when we stop loading it.

> 
> > 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 -nodefconfig -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.9 ChangeLog.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  vl.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/vl.c b/vl.c
> > index b563f9b924..e80b6da4bd 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2999,6 +2999,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;
> >  }

-- 
Eduardo

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Jan 18, 2017 at 05:04:45PM +0100, 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.
>> 
>> Can you explain why you don't warn on an empty qemu.conf?
>
> I didnt't want to show the warning to users that won't be
> affected by the change. I assume that users with empty qemu.conf
> files won't care when we stop loading it.

Makes some sense.

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

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

[Eduardo Habkost]

Ping?

I would like to include the warning on QEMU 2.9 (and I assume it
qualifies as post-soft-freeze material).

I am not sure I should go with a simpler solution and print the
warning even if the config file was empty. Opinions?


On Wed, Jan 18, 2017 at 11:56:22AM -0200, Eduardo Habkost wrote:
> 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.
> 
> This series is based on my machine-next branch, at:
>   https://github.com/ehabkost/qemu.git machine-next
> 
> Eduardo Habkost (2):
>   config: qemu_config_parse() return number of config groups
>   vl: Print warning when a default config file is loaded
> 
>  util/qemu-config.c | 15 +++++++--------
>  vl.c               |  6 ++++++
>  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> -- 
> 2.11.0.259.g40922b1
> 
> 

-- 
Eduardo