Re: [Qemu-devel] [PATCH v4 2/2] vl: Deprecate auto-loading of qemu.conf

[Eduardo Habkost <ehabkost@redhat.com>]

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