Re: [Qemu-devel] [PATCH] vl.c: Replace fprintf(stderr) with error_report()

[Eric Blake]

On 10/26/2015 11:13 AM, Eduardo Habkost wrote:
> This replaces most fprintf(stderr) calls on vl.c with error_report().
> 
> The trailing newlines, "qemu:" and "error:" message prefixes were
> removed.
> 
> The only remaining fprintf(stderr) calls are the ones at
> qemu_kill_report(), because the error mesage is split in multiple
> fprintf() calls.
> 
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
> Not sure if this is appropriate post soft-freeze, but if we are going to apply
> the max-cpus patch from Drew before 2.5.0, we could simply change all the
> fprintf() calls in a single step.

Soft freeze means no new features that didn't already have a patch
review started - but this is a cleanup, not a new feature. I think it
qualifies for 2.5 inclusion just fine.

> ---
>  vl.c | 228 
> +++++++++++++++++++++++++++++++++----------------------------------
>  1 file changed, 112 insertions(+), 116 deletions(-)

In addition to the things Markus pointed out,

> @@ -950,7 +950,7 @@ static struct bt_device_s *bt_device_add(const char *opt)
>      if (endp) {
>          vlan_id = strtol(endp + 6, &endp, 0);
>          if (*endp) {
> -            fprintf(stderr, "qemu: unrecognised bluetooth vlan Id\n");
> +            error_report("unrecognised bluetooth vlan Id");
>              return 0;

Do we have any strong preference for US vs. UK spelling in user-visible
messages?  (I don't, but someone else might)

>          }
>      }
> @@ -958,13 +958,13 @@ static struct bt_device_s *bt_device_add(const char 
> *opt)
>      vlan = qemu_find_bt_vlan(vlan_id);
>  
>      if (!vlan->slave)
> -        fprintf(stderr, "qemu: warning: adding a slave device to "
> -                        "an empty scatternet %i\n", vlan_id);
> +        error_report("warning: adding a slave device to "
> +                     "an empty scatternet %i", vlan_id);
>  
>      if (!strcmp(devname, "keyboard"))
>          return bt_keyboard_init(vlan);
>  
> -    fprintf(stderr, "qemu: unsupported bluetooth device `%s'\n", devname);
> +    error_report("unsupported bluetooth device `%s'", devname);

Use of `' quoting is unusual these days; if it were me, I'd use this as
a chance to switch to '' quoting.

> @@ -3005,8 +3006,7 @@ int main(int argc, char **argv, char **envp)
>      runstate_init();
>  
>      if (qcrypto_init(&err) < 0) {
> -        fprintf(stderr, "Cannot initialize crypto: %s\n",
> -                error_get_pretty(err));
> +        error_report("Cannot initialize crypto: %s", error_get_pretty(err));
>          exit(1);
>      }

error_report_err() is nicer than manually calling
error_report(error_get_pretty(err))

> @@ -3999,8 +3995,8 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      if (machine_class == NULL) {
> -        fprintf(stderr, "No machine specified, and there is no default.\n"
> -                "Use -machine help to list supported machines!\n");
> +        error_report("No machine specified, and there is no default.\n"
> +                "Use -machine help to list supported machines!");

Indentation looks off.

> @@ -4164,12 +4160,12 @@ int main(int argc, char **argv, char **envp)
>          if (display_type == DT_NOGRAPHIC
>              && (default_parallel || default_serial
>                  || default_monitor || default_virtcon)) {
> -            fprintf(stderr, "-nographic can not be used with -daemonize\n");
> +            error_report("-nographic can not be used with -daemonize");

s/can not/cannot/ while touching this

>              exit(1);
>          }
>  #ifdef CONFIG_CURSES
>          if (display_type == DT_CURSES) {
> -            fprintf(stderr, "curses display can not be used with 
> -daemonize\n");
> +            error_report("curses display can not be used with -daemonize");

and again

Whether you squash in those fixes, roll a v2, or send a followup, feel
free to add:

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature