[4/4] cpu model corrections/updates: add verbose config file handling

Failure by qemu to open a default config file isn't cause to
error exit -- it just quietly continues on.   After puzzling
issues with otherwise opaque config file locations and
startup handling numerous times, some help from qemu seemed
justified.

In the case of a "?" pseudo filename arg to -readconfig,
verbose open of all config files will be enabled.  Normal
handling of config files is otherwise unaffected by this
option.

Signed-off-by: john cooper <john.cooper@redhat.com>
---

On Tue, Sep 7, 2010 at 12:31 PM, john cooper <john.cooper@redhat.com> wrote:
> Failure by qemu to open a default config file isn't cause to
> error exit -- it just quietly continues on.   After puzzling
> issues with otherwise opaque config file locations and
> startup handling numerous times, some help from qemu seemed
> justified.

Maybe there should be an error exit if the user specifies a config
file but there are problems with it?

> In the case of a "?" pseudo filename arg to -readconfig,
> verbose open of all config files will be enabled.  Normal
> handling of config files is otherwise unaffected by this
> option.

I think '?' is not very good name. Could we add flags to -readconfig,
like -readconfig verbose,nodefaultconfig,file='', to match other
options' syntax?

> Signed-off-by: john cooper <john.cooper@redhat.com>
> ---
>
> diff --git a/qemu-config.c b/qemu-config.c
> index 1efdbec..805fcc6 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -534,21 +534,31 @@ out:
>     return res;
>  }
>
> -int qemu_read_config_file(const char *filename)
> +/* attempt to open and parse config file, report problems if vflag
> + */
> +int qemu_read_config_file(const char *filename, int vflag)
>  {
>     FILE *f = fopen(filename, "r");
> -    int ret;
> +    int rv = 0;
> +    const char *err;
>
>     if (f == NULL) {
> -        return -errno;
> +        rv = -errno;
> +        err = "open";
>     }
> -
> -    ret = qemu_config_parse(f, vm_config_groups, filename);
> -    fclose(f);
> -
> -    if (ret == 0) {
> -        return 0;
> -    } else {
> -        return -EINVAL;
> +    else if (qemu_config_parse(f, vm_config_groups, filename) != 0) {
> +        rv = -EINVAL;
> +        err = "parse";
> +    }
> +    else if (vflag) {
> +        fprintf(stderr, "parsed config file %s\n", filename);
> +    }
> +    if (f) {
> +        fclose(f);
> +    }
> +    if (rv && vflag) {
> +        fprintf(stderr, "can't %s config file %s: %s\n",
> +                err, filename, strerror(-rv));

I'd just duplicate this for both open and parse.

>     }
> +    return rv;
>  }
> diff --git a/qemu-config.h b/qemu-config.h
> index 533a049..897fbce 100644
> --- a/qemu-config.h
> +++ b/qemu-config.h
> @@ -13,6 +13,6 @@ void qemu_add_globals(void);
>  void qemu_config_write(FILE *fp);
>  int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname);
>
> -int qemu_read_config_file(const char *filename);
> +int qemu_read_config_file(const char *filename, int vflag);
>
>  #endif /* QEMU_CONFIG_H */
> diff --git a/vl.c b/vl.c
> index fd491ba..d192b51 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1822,6 +1822,7 @@ int main(int argc, char **argv, char **envp)
>     const char *incoming = NULL;
>     int show_vnc_port = 0;
>     int defconfig = 1;
> +    int defconfig_verbose = 0;
>
>     atexit(qemu_run_exit_notifiers);
>     error_set_progname(argv[0]);
> @@ -1875,6 +1876,11 @@ int main(int argc, char **argv, char **envp)
>             case QEMU_OPTION_nodefconfig:
>                 defconfig=0;
>                 break;
> +            case QEMU_OPTION_readconfig:
> +                /* pseudo filename "?" enables verbose config file handling */
> +                if (!strcmp(optarg, "?"))
> +                    defconfig_verbose = 1;
> +                break;
>             }
>         }
>     }
> @@ -1882,12 +1888,13 @@ int main(int argc, char **argv, char **envp)
>     if (defconfig) {
>         int ret;
>
> -        ret = qemu_read_config_file(CONFIG_QEMU_CONFDIR "/qemu.conf");
> +        ret = qemu_read_config_file(CONFIG_QEMU_CONFDIR "/qemu.conf",
> +                                    defconfig_verbose);
>         if (ret < 0 && ret != -ENOENT) {
>             exit(1);
>         }
>
> -        ret = qemu_read_config_file(arch_config_name);
> +        ret = qemu_read_config_file(arch_config_name, defconfig_verbose);
>         if (ret < 0 && ret != -ENOENT) {
>             exit(1);
>         }
> @@ -2596,15 +2603,10 @@ int main(int argc, char **argv, char **envp)
>                 xen_mode = XEN_ATTACH;
>                 break;
>             case QEMU_OPTION_readconfig:
> -                {
> -                    int ret = qemu_read_config_file(optarg);
> -                    if (ret < 0) {
> -                        fprintf(stderr, "read config %s: %s\n", optarg,
> -                            strerror(-ret));
> +                if (strcmp(optarg, "?") &&
> +                    qemu_read_config_file(optarg, defconfig_verbose) < 0)
>                         exit(1);
> -                    }
> -                    break;
> -                }
> +                break;
>             case QEMU_OPTION_writeconfig:
>                 {
>                     FILE *fp;
> --
> john.cooper@redhat.com
>
>
>

Blue Swirl wrote:
> On Tue, Sep 7, 2010 at 12:31 PM, john cooper <john.cooper@redhat.com> wrote:
>> Failure by qemu to open a default config file isn't cause to
>> error exit -- it just quietly continues on.   After puzzling
>> issues with otherwise opaque config file locations and
>> startup handling numerous times, some help from qemu seemed
>> justified.
> 
> Maybe there should be an error exit if the user specifies a config
> file but there are problems with it?

That's one possibility.  However given the preexisting
behavior where open of at least one of the config files
routinely fails and is quietly dismissed, issuing warnings
would seem distracting to the user.

I think one config file is all which is needed, and the
config syntax can be extended to allow including other
vendor/install specific files as needed.  I particularly
feel so as we've locally had to add yet a third config file
to push system quasi-static config data out of the way of
possible user modification for libvirt concerns.  That was
a last-minute bandaid solution which just makes the problem
worse.  Anyway such vendor specific config structure should
be handled within the config mechanism itself vs. hard coding
it into qemu startup.

>> In the case of a "?" pseudo filename arg to -readconfig,
>> verbose open of all config files will be enabled.  Normal
>> handling of config files is otherwise unaffected by this
>> option.
> 
> I think '?' is not very good name.

I agree, a shell meta char wasn't my first choice.  However
it follows the precedent of '?' used in similar query operations
and was chosen only for CLI consistency.
 
> Could we add flags to -readconfig,
> like -readconfig verbose,nodefaultconfig,file='', to match other
> options' syntax?

That seems most natural for options specific to the associated
config file.  However the verbose flag was intended as a
global action rather than local to a given config file.  The
preexisting "nodefconfig" is also a global option. 
 
Thanks,

-john

On Thu, Sep 9, 2010 at 3:48 AM, john cooper <john.cooper@redhat.com> wrote:
> Blue Swirl wrote:
>> On Tue, Sep 7, 2010 at 12:31 PM, john cooper <john.cooper@redhat.com> wrote:
>>> Failure by qemu to open a default config file isn't cause to
>>> error exit -- it just quietly continues on.   After puzzling
>>> issues with otherwise opaque config file locations and
>>> startup handling numerous times, some help from qemu seemed
>>> justified.
>>
>> Maybe there should be an error exit if the user specifies a config
>> file but there are problems with it?
>
> That's one possibility.  However given the preexisting
> behavior where open of at least one of the config files
> routinely fails and is quietly dismissed, issuing warnings
> would seem distracting to the user.
>
> I think one config file is all which is needed, and the
> config syntax can be extended to allow including other
> vendor/install specific files as needed.  I particularly
> feel so as we've locally had to add yet a third config file
> to push system quasi-static config data out of the way of
> possible user modification for libvirt concerns.  That was
> a last-minute bandaid solution which just makes the problem
> worse.  Anyway such vendor specific config structure should
> be handled within the config mechanism itself vs. hard coding
> it into qemu startup.
>
>>> In the case of a "?" pseudo filename arg to -readconfig,
>>> verbose open of all config files will be enabled.  Normal
>>> handling of config files is otherwise unaffected by this
>>> option.
>>
>> I think '?' is not very good name.
>
> I agree, a shell meta char wasn't my first choice.  However
> it follows the precedent of '?' used in similar query operations
> and was chosen only for CLI consistency.

But '?' is used for other purposes: query available options. It would
be more logical if -readconfig '?' instead could be used to query the
default config files.

>> Could we add flags to -readconfig,
>> like -readconfig verbose,nodefaultconfig,file='', to match other
>> options' syntax?
>
> That seems most natural for options specific to the associated
> config file.  However the verbose flag was intended as a
> global action rather than local to a given config file.  The
> preexisting "nodefconfig" is also a global option.

Right. It just seems that there are a lot of global flags. How about
-config nodefaults,verbose?

Blue Swirl wrote:
> On Thu, Sep 9, 2010 at 3:48 AM, john cooper <john.cooper@redhat.com> wrote:
>>
>>> I think '?' is not very good name.
>> I agree, a shell meta char wasn't my first choice.  However
>> it follows the precedent of '?' used in similar query operations
>> and was chosen only for CLI consistency.
>
> But '?' is used for other purposes: query available options. It would
> be more logical if -readconfig '?' instead could be used to query the
> default config files.
>
Which is what -writeconfig does, sort of.  Although the structure
of potential multiple input config files is lost and rather the
resulting flat config space is output.  But the alternative is
probably more of a headache than it's worth.

>>> Could we add flags to -readconfig,
>>> like -readconfig verbose,nodefaultconfig,file='', to match other
>>> options' syntax?
>> That seems most natural for options specific to the associated
>> config file.  However the verbose flag was intended as a
>> global action rather than local to a given config file.  The
>> preexisting "nodefconfig" is also a global option.
>
> Right. It just seems that there are a lot of global flags. How about
> -config nodefaults,verbose?
>   
Agreed this is more logical.  The reason I avoided doing so initially
was concern over impacting existing usage.  But it isn't really much
in retrospect.  Updated patch follows.

-john