diff mbox

vl.c: Replace fprintf(stderr) with error_report()

Message ID 1445879623-8403-1-git-send-email-ehabkost@redhat.com
State New
Headers show

Commit Message

Eduardo Habkost Oct. 26, 2015, 5:13 p.m. UTC
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 <ehabkost@redhat.com>
---
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.
---
 vl.c | 228 +++++++++++++++++++++++++++++++++----------------------------------
 1 file changed, 112 insertions(+), 116 deletions(-)

Comments

Markus Armbruster Oct. 26, 2015, 5:43 p.m. UTC | #1
Eduardo Habkost <ehabkost@redhat.com> writes:

> This replaces most fprintf(stderr) calls on vl.c with error_report().
>
> The trailing newlines, "qemu:" and "error:" message prefixes were
> removed.

Good!

Some of the messages end in a period, which could also be dropped.
Example:

    Usage: -virtfs fsdriver,mount_tag=tag.

> The only remaining fprintf(stderr) calls are the ones at
> qemu_kill_report(), because the error mesage is split in multiple
> fprintf() calls.

I think the straightforward solution would be two error_report() calls:

@@ -1624,14 +1624,14 @@ static int qemu_shutdown_requested(void)
 static void qemu_kill_report(void)
 {
     if (!qtest_driver() && shutdown_signal != -1) {
-        fprintf(stderr, "qemu: terminating on signal %d", shutdown_signal);
         if (shutdown_pid == 0) {
+            error_report("terminating on signal %d", shutdown_signal);
             /* This happens for eg ^C at the terminal, so it's worth
              * avoiding printing an odd message in that case.
              */
-            fputc('\n', stderr);
         } else {
-            fprintf(stderr, " from pid " FMT_pid "\n", shutdown_pid);
+            error_report("terminating on signal %d from pid" FMT_pid,
+                         shutdown_signal, shutdown_pid);
         }
         shutdown_signal = -1;
     }

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

I think it is just fine post soft freeze.  It's soft, not hard freeze.

Neither of my suggestions is important enough to deny my
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Eduardo Habkost Oct. 26, 2015, 6:03 p.m. UTC | #2
On Mon, Oct 26, 2015 at 06:43:43PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > This replaces most fprintf(stderr) calls on vl.c with error_report().
> >
> > The trailing newlines, "qemu:" and "error:" message prefixes were
> > removed.
> 
> Good!
> 
> Some of the messages end in a period, which could also be dropped.
> Example:
> 
>     Usage: -virtfs fsdriver,mount_tag=tag.

I will do that as a follow-up, or in case I have to send a new version.

> 
> > The only remaining fprintf(stderr) calls are the ones at
> > qemu_kill_report(), because the error mesage is split in multiple
> > fprintf() calls.
> 
> I think the straightforward solution would be two error_report() calls:
> 
> @@ -1624,14 +1624,14 @@ static int qemu_shutdown_requested(void)
>  static void qemu_kill_report(void)
>  {
>      if (!qtest_driver() && shutdown_signal != -1) {
> -        fprintf(stderr, "qemu: terminating on signal %d", shutdown_signal);
>          if (shutdown_pid == 0) {
> +            error_report("terminating on signal %d", shutdown_signal);
>              /* This happens for eg ^C at the terminal, so it's worth
>               * avoiding printing an odd message in that case.
>               */
> -            fputc('\n', stderr);
>          } else {
> -            fprintf(stderr, " from pid " FMT_pid "\n", shutdown_pid);
> +            error_report("terminating on signal %d from pid" FMT_pid,

Missing whitespace here. I will fix it and submit a patch.

> +                         shutdown_signal, shutdown_pid);
>          }
>          shutdown_signal = -1;
>      }
> 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > 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.
> 
> I think it is just fine post soft freeze.  It's soft, not hard freeze.
> 
> Neither of my suggestions is important enough to deny my
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!
Eduardo Habkost Oct. 26, 2015, 6:21 p.m. UTC | #3
On Mon, Oct 26, 2015 at 06:43:43PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
[...]
> > 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.
> 
> I think it is just fine post soft freeze.  It's soft, not hard freeze.

If there are no objections, I will wait for an additional Reviewed-by or
Acked-by, and submit a pull request through my x86 tree.
Eric Blake Oct. 26, 2015, 7:10 p.m. UTC | #4
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 <ehabkost@redhat.com>
> ---
> 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 <eblake@redhat.com>
Andrew Jones Oct. 27, 2015, 7:30 a.m. UTC | #5
On Mon, Oct 26, 2015 at 03:13:43PM -0200, 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 <ehabkost@redhat.com>
> ---
> 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.

In addition to Markus' and Eric's comments, I think we should

1. make sure the first word's case is correct. Lowercase for phrases,
   uppercase for sentences and proper nouns.
2. make sure the 'warning' prefix is consistent in all uses. This should
   be a QEMU-wide change. Maybe we need an error_report_warn variant?
3. make sure past tense is used in phrases like "failed to do..."
4. only break the line if necessary. Some of the lines are broken even
   though they could fit in 80 char. Probably the opposite exists too,
   i.e. some are > 80.
  
I've tried to point out a few of the cases below that inspired me to
to write these suggestions.

Thanks,
drew


> ---
>  vl.c | 228 +++++++++++++++++++++++++++++++++----------------------------------
>  1 file changed, 112 insertions(+), 116 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index dffaf09..db4a1f5 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -674,9 +674,9 @@ void runstate_set(RunState new_state)
>      assert(new_state < RUN_STATE_MAX);
>  
>      if (!runstate_valid_transitions[current_run_state][new_state]) {
> -        fprintf(stderr, "ERROR: invalid runstate transition: '%s' -> '%s'\n",
> -                RunState_lookup[current_run_state],
> -                RunState_lookup[new_state]);
> +        error_report("invalid runstate transition: '%s' -> '%s'",
> +                     RunState_lookup[current_run_state],
> +                     RunState_lookup[new_state]);
>          abort();
>      }
>      trace_runstate_set(new_state);
> @@ -828,8 +828,8 @@ static void configure_rtc_date_offset(const char *startdate, int legacy)
>          rtc_start_date = mktimegm(&tm);
>          if (rtc_start_date == -1) {
>          date_fail:
> -            fprintf(stderr, "Invalid date format. Valid formats are:\n"
> -                            "'2006-06-17T16:01:21' or '2006-06-17'\n");
> +            error_report("Invalid date format. Valid formats are:\n"
> +                         "'2006-06-17T16:01:21' or '2006-06-17'");
>              exit(1);
>          }
>          rtc_date_offset = qemu_time() - rtc_start_date;
> @@ -859,7 +859,7 @@ static void configure_rtc(QemuOpts *opts)
>          } else if (!strcmp(value, "vm")) {
>              rtc_clock = QEMU_CLOCK_VIRTUAL;
>          } else {
> -            fprintf(stderr, "qemu: invalid option value '%s'\n", value);
> +            error_report("invalid option value '%s'", value);
>              exit(1);
>          }
>      }
> @@ -879,7 +879,7 @@ static void configure_rtc(QemuOpts *opts)
>          } else if (!strcmp(value, "none")) {
>              /* discard is default */
>          } else {
> -            fprintf(stderr, "qemu: invalid option value '%s'\n", value);
> +            error_report("invalid option value '%s'", value);
>              exit(1);
>          }
>      }
> @@ -905,7 +905,7 @@ static int bt_hci_parse(const char *str)
>      bdaddr_t bdaddr;
>  
>      if (nb_hcis >= MAX_NICS) {
> -        fprintf(stderr, "qemu: Too many bluetooth HCIs (max %i).\n", MAX_NICS);
> +        error_report("Too many bluetooth HCIs (max %i).", MAX_NICS);
>          return -1;
>      }
>  
> @@ -931,8 +931,8 @@ static void bt_vhci_add(int vlan_id)
>      struct bt_scatternet_s *vlan = qemu_find_bt_vlan(vlan_id);
>  
>      if (!vlan->slave)
> -        fprintf(stderr, "qemu: warning: adding a VHCI to "
> -                        "an empty scatternet %i\n", vlan_id);
> +        error_report("warning: adding a VHCI to "
> +                     "an empty scatternet %i", vlan_id);
>  
>      bt_vhci_init(bt_new_hci(vlan));
>  }
> @@ -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;
>          }
>      }
> @@ -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);
>      return 0;
>  }
>  
> @@ -987,11 +987,11 @@ static int bt_parse(const char *opt)
>                  if (strstart(endp, ",vlan=", &p)) {
>                      vlan = strtol(p, (char **) &endp, 0);
>                      if (*endp) {
> -                        fprintf(stderr, "qemu: bad scatternet '%s'\n", p);
> +                        error_report("bad scatternet '%s'", p);
>                          return 1;
>                      }
>                  } else {
> -                    fprintf(stderr, "qemu: bad parameter '%s'\n", endp + 1);
> +                    error_report("bad parameter '%s'", endp + 1);
>                      return 1;
>                  }
>              } else
> @@ -1003,7 +1003,7 @@ static int bt_parse(const char *opt)
>      } else if (strstart(opt, "device:", &endp))
>          return !bt_device_add(endp);
>  
> -    fprintf(stderr, "qemu: bad bluetooth parameter '%s'\n", opt);
> +    error_report("bad bluetooth parameter '%s'", opt);
>      return 1;
>  }
>  
> @@ -1220,18 +1220,19 @@ static void smp_parse(QemuOpts *opts)
>          } else if (threads == 0) {
>              threads = cpus / (cores * sockets);
>          } else if (sockets * cores * threads < cpus) {
> -            fprintf(stderr, "cpu topology: error: "
> -                    "sockets (%u) * cores (%u) * threads (%u) < "
> -                    "smp_cpus (%u)\n",
> -                    sockets, cores, threads, cpus);
> +            error_report("cpu topology: "
> +                         "sockets (%u) * cores (%u) * threads (%u) < "
> +                         "smp_cpus (%u)",
> +                         sockets, cores, threads, cpus);
>              exit(1);
>          }
>  
>          max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
>          if (sockets * cores * threads > max_cpus) {
> -            fprintf(stderr, "cpu topology: error: "
> -                    "sockets (%u) * cores (%u) * threads (%u) > maxcpus (%u)\n",
> -                    sockets, cores, threads, max_cpus);
> +            error_report("cpu topology: "
> +                         "sockets (%u) * cores (%u) * threads (%u) > "
> +                         "maxcpus (%u)",
> +                         sockets, cores, threads, max_cpus);
>              exit(1);
>          }
>  
> @@ -1246,11 +1247,11 @@ static void smp_parse(QemuOpts *opts)
>      }
>  
>      if (max_cpus > MAX_CPUMASK_BITS) {
> -        fprintf(stderr, "Unsupported number of maxcpus\n");
> +        error_report("Unsupported number of maxcpus");

Unsupported should probably start with a small 'u'

>          exit(1);
>      }
>      if (max_cpus < smp_cpus) {
> -        fprintf(stderr, "maxcpus must be equal to or greater than smp\n");
> +        error_report("maxcpus must be equal to or greater than smp");
>          exit(1);
>      }
>  
> @@ -1260,7 +1261,7 @@ static void realtime_init(void)
>  {
>      if (enable_mlock) {
>          if (os_mlock() < 0) {
> -            fprintf(stderr, "qemu: locking memory failed\n");
> +            error_report("locking memory failed");
>              exit(1);
>          }
>      }
> @@ -1414,7 +1415,7 @@ static int usb_parse(const char *cmdline)
>      int r;
>      r = usb_device_add(cmdline);
>      if (r < 0) {
> -        fprintf(stderr, "qemu: could not add USB device '%s'\n", cmdline);
> +        error_report("could not add USB device '%s'", cmdline);
>      }
>      return r;
>  }
> @@ -1980,28 +1981,28 @@ static void select_vgahw (const char *p)
>          if (vga_available()) {
>              vga_interface_type = VGA_STD;
>          } else {
> -            fprintf(stderr, "Error: standard VGA not available\n");
> +            error_report("standard VGA not available");
>              exit(0);
>          }
>      } else if (strstart(p, "cirrus", &opts)) {
>          if (cirrus_vga_available()) {
>              vga_interface_type = VGA_CIRRUS;
>          } else {
> -            fprintf(stderr, "Error: Cirrus VGA not available\n");
> +            error_report("Cirrus VGA not available");
>              exit(0);
>          }
>      } else if (strstart(p, "vmware", &opts)) {
>          if (vmware_vga_available()) {
>              vga_interface_type = VGA_VMWARE;
>          } else {
> -            fprintf(stderr, "Error: VMWare SVGA not available\n");
> +            error_report("VMWare SVGA not available");
>              exit(0);
>          }
>      } else if (strstart(p, "virtio", &opts)) {
>          if (virtio_vga_available()) {
>              vga_interface_type = VGA_VIRTIO;
>          } else {
> -            fprintf(stderr, "Error: Virtio VGA not available\n");
> +            error_report("Virtio VGA not available");
>              exit(0);
>          }
>      } else if (strstart(p, "xenfb", &opts)) {
> @@ -2010,26 +2011,26 @@ static void select_vgahw (const char *p)
>          if (qxl_vga_available()) {
>              vga_interface_type = VGA_QXL;
>          } else {
> -            fprintf(stderr, "Error: QXL VGA not available\n");
> +            error_report("QXL VGA not available");
>              exit(0);
>          }
>      } else if (strstart(p, "tcx", &opts)) {
>          if (tcx_vga_available()) {
>              vga_interface_type = VGA_TCX;
>          } else {
> -            fprintf(stderr, "Error: TCX framebuffer not available\n");
> +            error_report("TCX framebuffer not available");
>              exit(0);
>          }
>      } else if (strstart(p, "cg3", &opts)) {
>          if (cg3_vga_available()) {
>              vga_interface_type = VGA_CG3;
>          } else {
> -            fprintf(stderr, "Error: CG3 framebuffer not available\n");
> +            error_report("CG3 framebuffer not available");
>              exit(0);
>          }
>      } else if (!strstart(p, "none", &opts)) {
>      invalid_vga:
> -        fprintf(stderr, "Unknown vga type: %s\n", p);
> +        error_report("Unknown vga type: %s", p);

lowercase u

>          exit(1);
>      }
>      while (*opts) {
> @@ -2349,7 +2350,7 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
>      } else if (strcmp(mode, "control") == 0) {
>          flags = MONITOR_USE_CONTROL;
>      } else {
> -        fprintf(stderr, "unknown monitor mode \"%s\"\n", mode);
> +        error_report("unknown monitor mode \"%s\"", mode);
>          exit(1);
>      }
>  
> @@ -2362,7 +2363,7 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
>      chardev = qemu_opt_get(opts, "chardev");
>      chr = qemu_chr_find(chardev);
>      if (chr == NULL) {
> -        fprintf(stderr, "chardev \"%s\" not found\n", chardev);
> +        error_report("chardev \"%s\" not found", chardev);
>          exit(1);
>      }
>  
> @@ -2390,7 +2391,7 @@ static void monitor_parse(const char *optarg, const char *mode, bool pretty)
>          }
>          opts = qemu_chr_parse_compat(label, optarg);
>          if (!opts) {
> -            fprintf(stderr, "parse error: %s\n", optarg);
> +            error_report("parse error: %s", optarg);
>              exit(1);
>          }
>      }
> @@ -2464,14 +2465,14 @@ static int serial_parse(const char *devname)
>      if (strcmp(devname, "none") == 0)
>          return 0;
>      if (index == MAX_SERIAL_PORTS) {
> -        fprintf(stderr, "qemu: too many serial ports\n");
> +        error_report("too many serial ports");
>          exit(1);
>      }
>      snprintf(label, sizeof(label), "serial%d", index);
>      serial_hds[index] = qemu_chr_new(label, devname, NULL);
>      if (!serial_hds[index]) {
> -        fprintf(stderr, "qemu: could not connect serial device"
> -                " to character backend '%s'\n", devname);
> +        error_report("could not connect serial device"
> +                     " to character backend '%s'", devname);
>          return -1;
>      }
>      index++;
> @@ -2486,14 +2487,14 @@ static int parallel_parse(const char *devname)
>      if (strcmp(devname, "none") == 0)
>          return 0;
>      if (index == MAX_PARALLEL_PORTS) {
> -        fprintf(stderr, "qemu: too many parallel ports\n");
> +        error_report("too many parallel ports");
>          exit(1);
>      }
>      snprintf(label, sizeof(label), "parallel%d", index);
>      parallel_hds[index] = qemu_chr_new(label, devname, NULL);
>      if (!parallel_hds[index]) {
> -        fprintf(stderr, "qemu: could not connect parallel device"
> -                " to character backend '%s'\n", devname);
> +        error_report("could not connect parallel device"
> +                     " to character backend '%s'", devname);
>          return -1;
>      }
>      index++;
> @@ -2510,7 +2511,7 @@ static int virtcon_parse(const char *devname)
>      if (strcmp(devname, "none") == 0)
>          return 0;
>      if (index == MAX_VIRTIO_CONSOLES) {
> -        fprintf(stderr, "qemu: too many virtio consoles\n");
> +        error_report("too many virtio consoles");
>          exit(1);
>      }
>  
> @@ -2527,8 +2528,8 @@ static int virtcon_parse(const char *devname)
>      snprintf(label, sizeof(label), "virtcon%d", index);
>      virtcon_hds[index] = qemu_chr_new(label, devname, NULL);
>      if (!virtcon_hds[index]) {
> -        fprintf(stderr, "qemu: could not connect virtio console"
> -                " to character backend '%s'\n", devname);
> +        error_report("could not connect virtio console"
> +                     " to character backend '%s'", devname);
>          return -1;
>      }
>      qemu_opt_set(dev_opts, "chardev", label, &error_abort);
> @@ -2548,7 +2549,7 @@ static int sclp_parse(const char *devname)
>          return 0;
>      }
>      if (index == MAX_SCLP_CONSOLES) {
> -        fprintf(stderr, "qemu: too many sclp consoles\n");
> +        error_report("too many sclp consoles");
>          exit(1);
>      }
>  
> @@ -2560,8 +2561,8 @@ static int sclp_parse(const char *devname)
>      snprintf(label, sizeof(label), "sclpcon%d", index);
>      sclp_hds[index] = qemu_chr_new(label, devname, NULL);
>      if (!sclp_hds[index]) {
> -        fprintf(stderr, "qemu: could not connect sclp console"
> -                " to character backend '%s'\n", devname);
> +        error_report("could not connect sclp console"
> +                     " to character backend '%s'", devname);
>          return -1;
>      }
>      qemu_opt_set(dev_opts, "chardev", label, &error_abort);
> @@ -2579,7 +2580,7 @@ static int debugcon_parse(const char *devname)
>      }
>      opts = qemu_opts_create(qemu_find_opts("device"), "debugcon", 1, NULL);
>      if (!opts) {
> -        fprintf(stderr, "qemu: already have a debugcon device\n");
> +        error_report("already have a debugcon device");
>          exit(1);
>      }
>      qemu_opt_set(opts, "driver", "isa-debugcon", &error_abort);
> @@ -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));

lowercase 'c'

>          exit(1);
>      }
>      rtc_clock = QEMU_CLOCK_HOST;
> @@ -3164,7 +3164,7 @@ int main(int argc, char **argv, char **envp)
>                          }
>                      } else if (*p != '\0') {
>                      chs_fail:
> -                        fprintf(stderr, "qemu: invalid physical CHS format\n");
> +                        error_report("invalid physical CHS format");
>                          exit(1);
>                      }
>                      if (hda_opts != NULL) {
> @@ -3207,7 +3207,7 @@ int main(int argc, char **argv, char **envp)
>  #ifdef CONFIG_CURSES
>                  display_type = DT_CURSES;
>  #else
> -                fprintf(stderr, "Curses support is disabled\n");
> +                error_report("Curses support is disabled");
>                  exit(1);
>  #endif
>                  break;
> @@ -3218,8 +3218,7 @@ int main(int argc, char **argv, char **envp)
>                  graphic_rotate = strtol(optarg, (char **) &optarg, 10);
>                  if (graphic_rotate != 0 && graphic_rotate != 90 &&
>                      graphic_rotate != 180 && graphic_rotate != 270) {
> -                    fprintf(stderr,
> -                        "qemu: only 90, 180, 270 deg rotation is available\n");
> +                    error_report("only 90, 180, 270 deg rotation is available");
>                      exit(1);
>                  }
>                  break;
> @@ -3370,7 +3369,7 @@ int main(int argc, char **argv, char **envp)
>                      w = strtol(p, (char **)&p, 10);
>                      if (w <= 0) {
>                      graphic_error:
> -                        fprintf(stderr, "qemu: invalid resolution or depth\n");
> +                        error_report("invalid resolution or depth");
>                          exit(1);
>                      }
>                      if (*p != 'x')
> @@ -3436,7 +3435,7 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_fsdev:
>                  olist = qemu_find_opts("fsdev");
>                  if (!olist) {
> -                    fprintf(stderr, "fsdev is not supported by this qemu build.\n");
> +                    error_report("fsdev is not supported by this qemu build.");
>                      exit(1);
>                  }
>                  opts = qemu_opts_parse_noisily(olist, optarg, true);
> @@ -3451,7 +3450,7 @@ int main(int argc, char **argv, char **envp)
>  
>                  olist = qemu_find_opts("virtfs");
>                  if (!olist) {
> -                    fprintf(stderr, "virtfs is not supported by this qemu build.\n");
> +                    error_report("virtfs is not supported by this qemu build.");
>                      exit(1);
>                  }
>                  opts = qemu_opts_parse_noisily(olist, optarg, true);
> @@ -3461,15 +3460,15 @@ int main(int argc, char **argv, char **envp)
>  
>                  if (qemu_opt_get(opts, "fsdriver") == NULL ||
>                      qemu_opt_get(opts, "mount_tag") == NULL) {
> -                    fprintf(stderr, "Usage: -virtfs fsdriver,mount_tag=tag.\n");
> +                    error_report("Usage: -virtfs fsdriver,mount_tag=tag.");

lowercase 'u' (remove period as suggested by Markus)

>                      exit(1);
>                  }
>                  fsdev = qemu_opts_create(qemu_find_opts("fsdev"),
>                                           qemu_opt_get(opts, "mount_tag"),
>                                           1, NULL);
>                  if (!fsdev) {
> -                    fprintf(stderr, "duplicate fsdev id: %s\n",
> -                            qemu_opt_get(opts, "mount_tag"));
> +                    error_report("duplicate fsdev id: %s",
> +                                 qemu_opt_get(opts, "mount_tag"));
>                      exit(1);
>                  }
>  
> @@ -3478,8 +3477,7 @@ int main(int argc, char **argv, char **envp)
>  #ifdef CONFIG_SYNC_FILE_RANGE
>                      qemu_opt_set(fsdev, "writeout", writeout, &error_abort);
>  #else
> -                    fprintf(stderr, "writeout=immediate not supported on "
> -                            "this platform\n");
> +                    error_report("writeout=immediate not supported on this platform");
>                      exit(1);
>  #endif
>                  }
> @@ -3518,7 +3516,7 @@ int main(int argc, char **argv, char **envp)
>                  fsdev = qemu_opts_create(qemu_find_opts("fsdev"), "v_synth",
>                                           1, NULL);
>                  if (!fsdev) {
> -                    fprintf(stderr, "duplicate option: %s\n", "virtfs_synth");
> +                    error_report("duplicate option: %s", "virtfs_synth");
>                      exit(1);
>                  }
>                  qemu_opt_set(fsdev, "fsdriver", "synth", &error_abort);
> @@ -3539,15 +3537,14 @@ int main(int argc, char **argv, char **envp)
>                  break;
>              case QEMU_OPTION_watchdog:
>                  if (watchdog) {
> -                    fprintf(stderr,
> -                            "qemu: only one watchdog option may be given\n");
> +                    error_report("only one watchdog option may be given");
>                      return 1;
>                  }
>                  watchdog = optarg;
>                  break;
>              case QEMU_OPTION_watchdog_action:
>                  if (select_watchdog_action(optarg) == -1) {
> -                    fprintf(stderr, "Unknown -watchdog-action parameter\n");
> +                    error_report("Unknown -watchdog-action parameter");

lowercase 'u'

>                      exit(1);
>                  }
>                  break;
> @@ -3591,7 +3588,7 @@ int main(int argc, char **argv, char **envp)
>                  display_type = DT_SDL;
>                  break;
>  #else
> -                fprintf(stderr, "SDL support is disabled\n");
> +                error_report("SDL support is disabled");
>                  exit(1);
>  #endif
>              case QEMU_OPTION_pidfile:
> @@ -3653,8 +3650,8 @@ int main(int argc, char **argv, char **envp)
>                  qemu_opts_parse_noisily(olist, "accel=tcg", false);
>                  break;
>              case QEMU_OPTION_no_kvm_pit: {
> -                fprintf(stderr, "Warning: KVM PIT can no longer be disabled "
> -                                "separately.\n");
> +                error_report("Warning: KVM PIT can no longer be disabled "
> +                             "separately.");

Could change this from a sentence into a phrase. Also, we need a consistent
'warning' prefix. Should we make a error_report_warn variant?

>                  break;
>              }
>              case QEMU_OPTION_no_kvm_pit_reinjection: {
> @@ -3667,8 +3664,8 @@ int main(int argc, char **argv, char **envp)
>                      { /* end of list */ }
>                  };
>  
> -                fprintf(stderr, "Warning: option deprecated, use "
> -                        "lost_tick_policy property of kvm-pit instead.\n");
> +                error_report("Warning: option deprecated, use "
> +                             "lost_tick_policy property of kvm-pit instead.");

same as last comment

>                  qdev_prop_register_global_list(kvm_pit_lost_tick_policy);
>                  break;
>              }
> @@ -3703,7 +3700,7 @@ int main(int argc, char **argv, char **envp)
>                      exit(1);
>                  }
>  #else
> -                fprintf(stderr, "VNC support is disabled\n");
> +                error_report("VNC support is disabled");
>                  exit(1);
>  #endif
>                  break;
> @@ -3716,7 +3713,7 @@ int main(int argc, char **argv, char **envp)
>                  break;
>              case QEMU_OPTION_balloon:
>                  if (balloon_parse(optarg) < 0) {
> -                    fprintf(stderr, "Unknown -balloon argument %s\n", optarg);
> +                    error_report("Unknown -balloon argument %s", optarg);

lowercase 'u'

>                      exit(1);
>                  }
>                  break;
> @@ -3731,15 +3728,15 @@ int main(int argc, char **argv, char **envp)
>                  break;
>              case QEMU_OPTION_uuid:
>                  if(qemu_uuid_parse(optarg, qemu_uuid) < 0) {
> -                    fprintf(stderr, "Fail to parse UUID string."
> -                            " Wrong format.\n");
> +                    error_report("Fail to parse UUID string."

should use past tense "Failed"

> +                                 " Wrong format.");
>                      exit(1);
>                  }
>                  qemu_uuid_set = true;
>                  break;
>              case QEMU_OPTION_option_rom:
>                  if (nb_option_roms >= MAX_OPTION_ROMS) {
> -                    fprintf(stderr, "Too many option ROMs\n");
> +                    error_report("Too many option ROMs");

lowercase 't'
>                      exit(1);
>                  }
>                  opts = qemu_opts_parse_noisily(qemu_find_opts("option-rom"),
> @@ -3751,7 +3748,7 @@ int main(int argc, char **argv, char **envp)
>                  option_rom[nb_option_roms].bootindex =
>                      qemu_opt_get_number(opts, "bootindex", -1);
>                  if (!option_rom[nb_option_roms].name) {
> -                    fprintf(stderr, "Option ROM file is not specified\n");
> +                    error_report("Option ROM file is not specified");
lowercase 'o'
>                      exit(1);
>                  }
>                  nb_option_roms++;
> @@ -3776,9 +3773,8 @@ int main(int argc, char **argv, char **envp)
>                          } else  if (strcmp("auto", target) == 0) {
>                              semihosting.target = SEMIHOSTING_TARGET_AUTO;
>                          } else {
> -                            fprintf(stderr, "Unsupported semihosting-config"
> -                                    " %s\n",
> -                                optarg);
> +                            error_report("Unsupported semihosting-config %s",
lowercase 'u'
> +                                         optarg);
>                              exit(1);
>                          }
>                      } else {
> @@ -3788,14 +3784,14 @@ int main(int argc, char **argv, char **envp)
>                      qemu_opt_foreach(opts, add_semihosting_arg,
>                                       &semihosting, NULL);
>                  } else {
> -                    fprintf(stderr, "Unsupported semihosting-config %s\n",
> -                            optarg);
> +                    error_report("Unsupported semihosting-config %s",
lowercase 'u'
> +                                 optarg);
>                      exit(1);
>                  }
>                  break;
>              case QEMU_OPTION_tdf:
> -                fprintf(stderr, "Warning: user space PIT time drift fix "
> -                                "is no longer supported.\n");
> +                error_report("Warning: user space PIT time drift fix "
> +                             "is no longer supported.");

no period and "error_report_warn"

>                  break;
>              case QEMU_OPTION_name:
>                  opts = qemu_opts_parse_noisily(qemu_find_opts("name"),
> @@ -3806,7 +3802,7 @@ int main(int argc, char **argv, char **envp)
>                  break;
>              case QEMU_OPTION_prom_env:
>                  if (nb_prom_envs >= MAX_PROM_ENVS) {
> -                    fprintf(stderr, "Too many prom variables\n");
> +                    error_report("Too many prom variables");

lowercase 't'

>                      exit(1);
>                  }
>                  prom_envs[nb_prom_envs] = optarg;
> @@ -3889,8 +3885,8 @@ int main(int argc, char **argv, char **envp)
>                  {
>                      int ret = qemu_read_config_file(optarg);
>                      if (ret < 0) {
> -                        fprintf(stderr, "read config %s: %s\n", optarg,
> -                            strerror(-ret));
> +                        error_report("read config %s: %s", optarg,
> +                                     strerror(-ret));
>                          exit(1);
>                      }
>                      break;
> @@ -3898,7 +3894,7 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_spice:
>                  olist = qemu_find_opts("spice");
>                  if (!olist) {
> -                    fprintf(stderr, "spice is not supported by this qemu build.\n");
> +                    error_report("spice is not supported by this qemu build.");
no period
>                      exit(1);
>                  }
>                  opts = qemu_opts_parse_noisily(olist, optarg, false);
> @@ -3915,7 +3911,7 @@ int main(int argc, char **argv, char **envp)
>                      } else {
>                          fp = fopen(optarg, "w");
>                          if (fp == NULL) {
> -                            fprintf(stderr, "open %s: %s\n", optarg, strerror(errno));
> +                            error_report("open %s: %s", optarg, strerror(errno));
>                              exit(1);
>                          }
>                      }
> @@ -3976,13 +3972,13 @@ int main(int argc, char **argv, char **envp)
>                  break;
>              case QEMU_OPTION_dump_vmstate:
>                  if (vmstate_dump_file) {
> -                    fprintf(stderr, "qemu: only one '-dump-vmstate' "
> -                            "option may be given\n");
> +                    error_report("only one '-dump-vmstate' "
> +                                 "option may be given");
>                      exit(1);
>                  }
>                  vmstate_dump_file = fopen(optarg, "w");
>                  if (vmstate_dump_file == NULL) {
> -                    fprintf(stderr, "open %s: %s\n", optarg, strerror(errno));
> +                    error_report("open %s: %s", optarg, strerror(errno));
>                      exit(1);
>                  }
>                  break;
> @@ -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!");
>          exit(1);
>      }
>  
> @@ -4101,9 +4097,9 @@ int main(int argc, char **argv, char **envp)
>  
>      machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */
>      if (max_cpus > machine_class->max_cpus) {
> -        fprintf(stderr, "Number of SMP CPUs requested (%d) exceeds max CPUs "
> -                "supported by machine '%s' (%d)\n", max_cpus,
> -                machine_class->name, machine_class->max_cpus);
> +        error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
> +                     "supported by machine '%s' (%d)", max_cpus,
> +                     machine_class->name, machine_class->max_cpus);
>          exit(1);
>      }
>  
> @@ -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");
>              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");

Capital 'C' for the proper noun 'Curses' ?

>              exit(1);
>          }
>  #endif
> @@ -4228,12 +4224,12 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      if ((no_frame || alt_grab || ctrl_grab) && display_type != DT_SDL) {
> -        fprintf(stderr, "-no-frame, -alt-grab and -ctrl-grab are only valid "
> -                        "for SDL, ignoring option\n");
> +        error_report("-no-frame, -alt-grab and -ctrl-grab are only valid "
> +                     "for SDL, ignoring option");
>      }
>      if (no_quit && (display_type != DT_GTK && display_type != DT_SDL)) {
> -        fprintf(stderr, "-no-quit is only valid for GTK and SDL, "
> -                        "ignoring option\n");
> +        error_report("-no-quit is only valid for GTK and SDL, "
> +                     "ignoring option");
>      }
>  
>  #if defined(CONFIG_GTK)
> @@ -4248,9 +4244,9 @@ int main(int argc, char **argv, char **envp)
>  #endif
>      if (request_opengl == 1 && display_opengl == 0) {
>  #if defined(CONFIG_OPENGL)
> -        fprintf(stderr, "OpenGL is not supported by the display.\n");
> +        error_report("OpenGL is not supported by the display.");
>  #else
> -        fprintf(stderr, "QEMU was built without opengl support.\n");
> +        error_report("QEMU was built without opengl support.");
>  #endif
>          exit(1);
>      }
> @@ -4276,7 +4272,7 @@ int main(int argc, char **argv, char **envp)
>  #endif
>  
>      if (pid_file && qemu_create_pidfile(pid_file) != 0) {
> -        fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
> +        error_report("Could not acquire pid file: %s", strerror(errno));
lowercase 'c'
>          exit(1);
>      }
>  
> @@ -4347,17 +4343,17 @@ int main(int argc, char **argv, char **envp)
>      linux_boot = (kernel_filename != NULL);
>  
>      if (!linux_boot && *kernel_cmdline != '\0') {
> -        fprintf(stderr, "-append only allowed with -kernel option\n");
> +        error_report("-append only allowed with -kernel option");
>          exit(1);
>      }
>  
>      if (!linux_boot && initrd_filename != NULL) {
> -        fprintf(stderr, "-initrd only allowed with -kernel option\n");
> +        error_report("-initrd only allowed with -kernel option");
>          exit(1);
>      }
>  
>      if (!linux_boot && qemu_opt_get(machine_opts, "dtb")) {
> -        fprintf(stderr, "-dtb only allowed with -kernel option\n");
> +        error_report("-dtb only allowed with -kernel option");
>          exit(1);
>      }
>  
> @@ -4376,7 +4372,7 @@ int main(int argc, char **argv, char **envp)
>      cpu_ticks_init();
>      if (icount_opts) {
>          if (kvm_enabled() || xen_enabled()) {
> -            fprintf(stderr, "-icount is not allowed with kvm or xen\n");
> +            error_report("-icount is not allowed with kvm or xen");
>              exit(1);
>          }
>          configure_icount(icount_opts, &error_abort);
> @@ -4409,7 +4405,7 @@ int main(int argc, char **argv, char **envp)
>      if (!xen_enabled()) {
>          /* On 32-bit hosts, QEMU is limited by virtual address space */
>          if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> -            fprintf(stderr, "qemu: at most 2047 MB RAM can be simulated\n");
> +            error_report("at most 2047 MB RAM can be simulated");
>              exit(1);
>          }
>      }
> @@ -4596,7 +4592,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_run_machine_init_done_notifiers();
>  
>      if (rom_check_and_register_reset() != 0) {
> -        fprintf(stderr, "rom check and register reset failed\n");
> +        error_report("rom check and register reset failed");
>          exit(1);
>      }
>  
> -- 
> 2.1.0
> 
>
Markus Armbruster Oct. 27, 2015, 9:53 a.m. UTC | #6
Andrew Jones <drjones@redhat.com> writes:

> On Mon, Oct 26, 2015 at 03:13:43PM -0200, 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 <ehabkost@redhat.com>
>> ---
>> 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.
>
> In addition to Markus' and Eric's comments, I think we should
>
> 1. make sure the first word's case is correct. Lowercase for phrases,
>    uppercase for sentences and proper nouns.
> 2. make sure the 'warning' prefix is consistent in all uses. This should
>    be a QEMU-wide change. Maybe we need an error_report_warn variant?
> 3. make sure past tense is used in phrases like "failed to do..."
> 4. only break the line if necessary. Some of the lines are broken even
>    though they could fit in 80 char. Probably the opposite exists too,
>    i.e. some are > 80.
>   
> I've tried to point out a few of the cases below that inspired me to
> to write these suggestions.

These rules all make sense to me.

Cleaning up existing error messages to conform to them is a lot of grunt
work.  Patches welcome.

In theory, making new error messages conform is a matter of patch
review.  In practice, I'm afraid it'll take written guidelines and at
least a local scarcity of bad examples to make patch review work even
moderately well there.

We discussed written guidelines in the "Coding style for errors" thread.
We need someone to synthesize it into a patch.
Eduardo Habkost Oct. 27, 2015, 5:08 p.m. UTC | #7
On Mon, Oct 26, 2015 at 01:10:31PM -0600, Eric Blake wrote:
> 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 <ehabkost@redhat.com>
> > ---
> > 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.

Makes sense. Thanks for the input!

> 
> > ---
> >  vl.c | 228 +++++++++++++++++++++++++++++++++----------------------------------
> >  1 file changed, 112 insertions(+), 116 deletions(-)
> 
> In addition to the things Markus pointed out,
> 
[many suggestions]
> 
> Whether you squash in those fixes, roll a v2, or send a followup, feel
> free to add:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Except for the indentation fix (which I already fixed in my git tree), I
will send them as a follow-up. Thanks!
Eduardo Habkost Oct. 27, 2015, 7:25 p.m. UTC | #8
On Tue, Oct 27, 2015 at 08:30:38AM +0100, Andrew Jones wrote:
> In addition to Markus' and Eric's comments, I think we should
> 
> 1. make sure the first word's case is correct. Lowercase for phrases,
>    uppercase for sentences and proper nouns.

I think I can understand this in the more obvious cases, but I don't
know how I could convert the following instance:

[...]
> >              case QEMU_OPTION_no_kvm_pit: {
> > -                fprintf(stderr, "Warning: KVM PIT can no longer be disabled "
> > -                                "separately.\n");
> > +                error_report("Warning: KVM PIT can no longer be disabled "
> > +                             "separately.");
> 
> Could change this from a sentence into a phrase. Also, we need a consistent
> 'warning' prefix. Should we make a error_report_warn variant?

Converting that sentence to a phrase is beyond my non-native english
speaker skills. Do you have any suggestions? :)
Laszlo Ersek Oct. 27, 2015, 7:36 p.m. UTC | #9
On 10/27/15 20:25, Eduardo Habkost wrote:
> On Tue, Oct 27, 2015 at 08:30:38AM +0100, Andrew Jones wrote:
>> In addition to Markus' and Eric's comments, I think we should
>>
>> 1. make sure the first word's case is correct. Lowercase for phrases,
>>    uppercase for sentences and proper nouns.
> 
> I think I can understand this in the more obvious cases, but I don't
> know how I could convert the following instance:
> 
> [...]
>>>              case QEMU_OPTION_no_kvm_pit: {
>>> -                fprintf(stderr, "Warning: KVM PIT can no longer be disabled "
>>> -                                "separately.\n");
>>> +                error_report("Warning: KVM PIT can no longer be disabled "
>>> +                             "separately.");
>>
>> Could change this from a sentence into a phrase. Also, we need a consistent
>> 'warning' prefix. Should we make a error_report_warn variant?
> 
> Converting that sentence to a phrase is beyond my non-native english
> speaker skills. Do you have any suggestions? :)
> 

easy-peasy, "KVM PIT no longer disableable separately"!

/me hides ;)
Andrew Jones Oct. 28, 2015, 1:58 p.m. UTC | #10
On Tue, Oct 27, 2015 at 08:36:07PM +0100, Laszlo Ersek wrote:
> On 10/27/15 20:25, Eduardo Habkost wrote:
> > On Tue, Oct 27, 2015 at 08:30:38AM +0100, Andrew Jones wrote:
> >> In addition to Markus' and Eric's comments, I think we should
> >>
> >> 1. make sure the first word's case is correct. Lowercase for phrases,
> >>    uppercase for sentences and proper nouns.
> > 
> > I think I can understand this in the more obvious cases, but I don't
> > know how I could convert the following instance:
> > 
> > [...]
> >>>              case QEMU_OPTION_no_kvm_pit: {
> >>> -                fprintf(stderr, "Warning: KVM PIT can no longer be disabled "
> >>> -                                "separately.\n");
> >>> +                error_report("Warning: KVM PIT can no longer be disabled "
> >>> +                             "separately.");
> >>
> >> Could change this from a sentence into a phrase. Also, we need a consistent
> >> 'warning' prefix. Should we make a error_report_warn variant?
> > 
> > Converting that sentence to a phrase is beyond my non-native english
> > speaker skills. Do you have any suggestions? :)
> > 
> 
> easy-peasy, "KVM PIT no longer disableable separately"!
> 
> /me hides ;)
As well you should with a word like "disableable" :-)

How about

  Warning: ignoring deprecated option no-kvm-pit

drew

>
Laszlo Ersek Oct. 28, 2015, 3:27 p.m. UTC | #11
On 10/28/15 14:58, Andrew Jones wrote:
> On Tue, Oct 27, 2015 at 08:36:07PM +0100, Laszlo Ersek wrote:
>> On 10/27/15 20:25, Eduardo Habkost wrote:
>>> On Tue, Oct 27, 2015 at 08:30:38AM +0100, Andrew Jones wrote:
>>>> In addition to Markus' and Eric's comments, I think we should
>>>>
>>>> 1. make sure the first word's case is correct. Lowercase for phrases,
>>>>    uppercase for sentences and proper nouns.
>>>
>>> I think I can understand this in the more obvious cases, but I don't
>>> know how I could convert the following instance:
>>>
>>> [...]
>>>>>              case QEMU_OPTION_no_kvm_pit: {
>>>>> -                fprintf(stderr, "Warning: KVM PIT can no longer be disabled "
>>>>> -                                "separately.\n");
>>>>> +                error_report("Warning: KVM PIT can no longer be disabled "
>>>>> +                             "separately.");
>>>>
>>>> Could change this from a sentence into a phrase. Also, we need a consistent
>>>> 'warning' prefix. Should we make a error_report_warn variant?
>>>
>>> Converting that sentence to a phrase is beyond my non-native english
>>> speaker skills. Do you have any suggestions? :)
>>>
>>
>> easy-peasy, "KVM PIT no longer disableable separately"!
>>
>> /me hides ;)
> As well you should with a word like "disableable" :-)
> 
> How about
> 
>   Warning: ignoring deprecated option no-kvm-pit

heh, side-stepping the problem semantically; that's a trick I'll steal! :)

> 
> drew
> 
>>
Eduardo Habkost Oct. 28, 2015, 6:12 p.m. UTC | #12
On Mon, Oct 26, 2015 at 04:21:16PM -0200, Eduardo Habkost wrote:
> On Mon, Oct 26, 2015 at 06:43:43PM +0100, Markus Armbruster wrote:
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> [...]
> > > 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.
> > 
> > I think it is just fine post soft freeze.  It's soft, not hard freeze.
> 
> If there are no objections, I will wait for an additional Reviewed-by or
> Acked-by, and submit a pull request through my x86 tree.

I have just noticed that we have a error reporting maintainer in
MAINTAINERS. :)

Markus, would you merge this (and the follow-up patches I will send)
through your tree?
Markus Armbruster Oct. 29, 2015, 6:15 a.m. UTC | #13
Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Oct 26, 2015 at 04:21:16PM -0200, Eduardo Habkost wrote:
>> On Mon, Oct 26, 2015 at 06:43:43PM +0100, Markus Armbruster wrote:
>> > Eduardo Habkost <ehabkost@redhat.com> writes:
>> [...]
>> > > 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.
>> > 
>> > I think it is just fine post soft freeze.  It's soft, not hard freeze.
>> 
>> If there are no objections, I will wait for an additional Reviewed-by or
>> Acked-by, and submit a pull request through my x86 tree.
>
> I have just noticed that we have a error reporting maintainer in
> MAINTAINERS. :)
>
> Markus, would you merge this (and the follow-up patches I will send)
> through your tree?

Can do.
diff mbox

Patch

diff --git a/vl.c b/vl.c
index dffaf09..db4a1f5 100644
--- a/vl.c
+++ b/vl.c
@@ -674,9 +674,9 @@  void runstate_set(RunState new_state)
     assert(new_state < RUN_STATE_MAX);
 
     if (!runstate_valid_transitions[current_run_state][new_state]) {
-        fprintf(stderr, "ERROR: invalid runstate transition: '%s' -> '%s'\n",
-                RunState_lookup[current_run_state],
-                RunState_lookup[new_state]);
+        error_report("invalid runstate transition: '%s' -> '%s'",
+                     RunState_lookup[current_run_state],
+                     RunState_lookup[new_state]);
         abort();
     }
     trace_runstate_set(new_state);
@@ -828,8 +828,8 @@  static void configure_rtc_date_offset(const char *startdate, int legacy)
         rtc_start_date = mktimegm(&tm);
         if (rtc_start_date == -1) {
         date_fail:
-            fprintf(stderr, "Invalid date format. Valid formats are:\n"
-                            "'2006-06-17T16:01:21' or '2006-06-17'\n");
+            error_report("Invalid date format. Valid formats are:\n"
+                         "'2006-06-17T16:01:21' or '2006-06-17'");
             exit(1);
         }
         rtc_date_offset = qemu_time() - rtc_start_date;
@@ -859,7 +859,7 @@  static void configure_rtc(QemuOpts *opts)
         } else if (!strcmp(value, "vm")) {
             rtc_clock = QEMU_CLOCK_VIRTUAL;
         } else {
-            fprintf(stderr, "qemu: invalid option value '%s'\n", value);
+            error_report("invalid option value '%s'", value);
             exit(1);
         }
     }
@@ -879,7 +879,7 @@  static void configure_rtc(QemuOpts *opts)
         } else if (!strcmp(value, "none")) {
             /* discard is default */
         } else {
-            fprintf(stderr, "qemu: invalid option value '%s'\n", value);
+            error_report("invalid option value '%s'", value);
             exit(1);
         }
     }
@@ -905,7 +905,7 @@  static int bt_hci_parse(const char *str)
     bdaddr_t bdaddr;
 
     if (nb_hcis >= MAX_NICS) {
-        fprintf(stderr, "qemu: Too many bluetooth HCIs (max %i).\n", MAX_NICS);
+        error_report("Too many bluetooth HCIs (max %i).", MAX_NICS);
         return -1;
     }
 
@@ -931,8 +931,8 @@  static void bt_vhci_add(int vlan_id)
     struct bt_scatternet_s *vlan = qemu_find_bt_vlan(vlan_id);
 
     if (!vlan->slave)
-        fprintf(stderr, "qemu: warning: adding a VHCI to "
-                        "an empty scatternet %i\n", vlan_id);
+        error_report("warning: adding a VHCI to "
+                     "an empty scatternet %i", vlan_id);
 
     bt_vhci_init(bt_new_hci(vlan));
 }
@@ -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;
         }
     }
@@ -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);
     return 0;
 }
 
@@ -987,11 +987,11 @@  static int bt_parse(const char *opt)
                 if (strstart(endp, ",vlan=", &p)) {
                     vlan = strtol(p, (char **) &endp, 0);
                     if (*endp) {
-                        fprintf(stderr, "qemu: bad scatternet '%s'\n", p);
+                        error_report("bad scatternet '%s'", p);
                         return 1;
                     }
                 } else {
-                    fprintf(stderr, "qemu: bad parameter '%s'\n", endp + 1);
+                    error_report("bad parameter '%s'", endp + 1);
                     return 1;
                 }
             } else
@@ -1003,7 +1003,7 @@  static int bt_parse(const char *opt)
     } else if (strstart(opt, "device:", &endp))
         return !bt_device_add(endp);
 
-    fprintf(stderr, "qemu: bad bluetooth parameter '%s'\n", opt);
+    error_report("bad bluetooth parameter '%s'", opt);
     return 1;
 }
 
@@ -1220,18 +1220,19 @@  static void smp_parse(QemuOpts *opts)
         } else if (threads == 0) {
             threads = cpus / (cores * sockets);
         } else if (sockets * cores * threads < cpus) {
-            fprintf(stderr, "cpu topology: error: "
-                    "sockets (%u) * cores (%u) * threads (%u) < "
-                    "smp_cpus (%u)\n",
-                    sockets, cores, threads, cpus);
+            error_report("cpu topology: "
+                         "sockets (%u) * cores (%u) * threads (%u) < "
+                         "smp_cpus (%u)",
+                         sockets, cores, threads, cpus);
             exit(1);
         }
 
         max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
         if (sockets * cores * threads > max_cpus) {
-            fprintf(stderr, "cpu topology: error: "
-                    "sockets (%u) * cores (%u) * threads (%u) > maxcpus (%u)\n",
-                    sockets, cores, threads, max_cpus);
+            error_report("cpu topology: "
+                         "sockets (%u) * cores (%u) * threads (%u) > "
+                         "maxcpus (%u)",
+                         sockets, cores, threads, max_cpus);
             exit(1);
         }
 
@@ -1246,11 +1247,11 @@  static void smp_parse(QemuOpts *opts)
     }
 
     if (max_cpus > MAX_CPUMASK_BITS) {
-        fprintf(stderr, "Unsupported number of maxcpus\n");
+        error_report("Unsupported number of maxcpus");
         exit(1);
     }
     if (max_cpus < smp_cpus) {
-        fprintf(stderr, "maxcpus must be equal to or greater than smp\n");
+        error_report("maxcpus must be equal to or greater than smp");
         exit(1);
     }
 
@@ -1260,7 +1261,7 @@  static void realtime_init(void)
 {
     if (enable_mlock) {
         if (os_mlock() < 0) {
-            fprintf(stderr, "qemu: locking memory failed\n");
+            error_report("locking memory failed");
             exit(1);
         }
     }
@@ -1414,7 +1415,7 @@  static int usb_parse(const char *cmdline)
     int r;
     r = usb_device_add(cmdline);
     if (r < 0) {
-        fprintf(stderr, "qemu: could not add USB device '%s'\n", cmdline);
+        error_report("could not add USB device '%s'", cmdline);
     }
     return r;
 }
@@ -1980,28 +1981,28 @@  static void select_vgahw (const char *p)
         if (vga_available()) {
             vga_interface_type = VGA_STD;
         } else {
-            fprintf(stderr, "Error: standard VGA not available\n");
+            error_report("standard VGA not available");
             exit(0);
         }
     } else if (strstart(p, "cirrus", &opts)) {
         if (cirrus_vga_available()) {
             vga_interface_type = VGA_CIRRUS;
         } else {
-            fprintf(stderr, "Error: Cirrus VGA not available\n");
+            error_report("Cirrus VGA not available");
             exit(0);
         }
     } else if (strstart(p, "vmware", &opts)) {
         if (vmware_vga_available()) {
             vga_interface_type = VGA_VMWARE;
         } else {
-            fprintf(stderr, "Error: VMWare SVGA not available\n");
+            error_report("VMWare SVGA not available");
             exit(0);
         }
     } else if (strstart(p, "virtio", &opts)) {
         if (virtio_vga_available()) {
             vga_interface_type = VGA_VIRTIO;
         } else {
-            fprintf(stderr, "Error: Virtio VGA not available\n");
+            error_report("Virtio VGA not available");
             exit(0);
         }
     } else if (strstart(p, "xenfb", &opts)) {
@@ -2010,26 +2011,26 @@  static void select_vgahw (const char *p)
         if (qxl_vga_available()) {
             vga_interface_type = VGA_QXL;
         } else {
-            fprintf(stderr, "Error: QXL VGA not available\n");
+            error_report("QXL VGA not available");
             exit(0);
         }
     } else if (strstart(p, "tcx", &opts)) {
         if (tcx_vga_available()) {
             vga_interface_type = VGA_TCX;
         } else {
-            fprintf(stderr, "Error: TCX framebuffer not available\n");
+            error_report("TCX framebuffer not available");
             exit(0);
         }
     } else if (strstart(p, "cg3", &opts)) {
         if (cg3_vga_available()) {
             vga_interface_type = VGA_CG3;
         } else {
-            fprintf(stderr, "Error: CG3 framebuffer not available\n");
+            error_report("CG3 framebuffer not available");
             exit(0);
         }
     } else if (!strstart(p, "none", &opts)) {
     invalid_vga:
-        fprintf(stderr, "Unknown vga type: %s\n", p);
+        error_report("Unknown vga type: %s", p);
         exit(1);
     }
     while (*opts) {
@@ -2349,7 +2350,7 @@  static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
     } else if (strcmp(mode, "control") == 0) {
         flags = MONITOR_USE_CONTROL;
     } else {
-        fprintf(stderr, "unknown monitor mode \"%s\"\n", mode);
+        error_report("unknown monitor mode \"%s\"", mode);
         exit(1);
     }
 
@@ -2362,7 +2363,7 @@  static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
     chardev = qemu_opt_get(opts, "chardev");
     chr = qemu_chr_find(chardev);
     if (chr == NULL) {
-        fprintf(stderr, "chardev \"%s\" not found\n", chardev);
+        error_report("chardev \"%s\" not found", chardev);
         exit(1);
     }
 
@@ -2390,7 +2391,7 @@  static void monitor_parse(const char *optarg, const char *mode, bool pretty)
         }
         opts = qemu_chr_parse_compat(label, optarg);
         if (!opts) {
-            fprintf(stderr, "parse error: %s\n", optarg);
+            error_report("parse error: %s", optarg);
             exit(1);
         }
     }
@@ -2464,14 +2465,14 @@  static int serial_parse(const char *devname)
     if (strcmp(devname, "none") == 0)
         return 0;
     if (index == MAX_SERIAL_PORTS) {
-        fprintf(stderr, "qemu: too many serial ports\n");
+        error_report("too many serial ports");
         exit(1);
     }
     snprintf(label, sizeof(label), "serial%d", index);
     serial_hds[index] = qemu_chr_new(label, devname, NULL);
     if (!serial_hds[index]) {
-        fprintf(stderr, "qemu: could not connect serial device"
-                " to character backend '%s'\n", devname);
+        error_report("could not connect serial device"
+                     " to character backend '%s'", devname);
         return -1;
     }
     index++;
@@ -2486,14 +2487,14 @@  static int parallel_parse(const char *devname)
     if (strcmp(devname, "none") == 0)
         return 0;
     if (index == MAX_PARALLEL_PORTS) {
-        fprintf(stderr, "qemu: too many parallel ports\n");
+        error_report("too many parallel ports");
         exit(1);
     }
     snprintf(label, sizeof(label), "parallel%d", index);
     parallel_hds[index] = qemu_chr_new(label, devname, NULL);
     if (!parallel_hds[index]) {
-        fprintf(stderr, "qemu: could not connect parallel device"
-                " to character backend '%s'\n", devname);
+        error_report("could not connect parallel device"
+                     " to character backend '%s'", devname);
         return -1;
     }
     index++;
@@ -2510,7 +2511,7 @@  static int virtcon_parse(const char *devname)
     if (strcmp(devname, "none") == 0)
         return 0;
     if (index == MAX_VIRTIO_CONSOLES) {
-        fprintf(stderr, "qemu: too many virtio consoles\n");
+        error_report("too many virtio consoles");
         exit(1);
     }
 
@@ -2527,8 +2528,8 @@  static int virtcon_parse(const char *devname)
     snprintf(label, sizeof(label), "virtcon%d", index);
     virtcon_hds[index] = qemu_chr_new(label, devname, NULL);
     if (!virtcon_hds[index]) {
-        fprintf(stderr, "qemu: could not connect virtio console"
-                " to character backend '%s'\n", devname);
+        error_report("could not connect virtio console"
+                     " to character backend '%s'", devname);
         return -1;
     }
     qemu_opt_set(dev_opts, "chardev", label, &error_abort);
@@ -2548,7 +2549,7 @@  static int sclp_parse(const char *devname)
         return 0;
     }
     if (index == MAX_SCLP_CONSOLES) {
-        fprintf(stderr, "qemu: too many sclp consoles\n");
+        error_report("too many sclp consoles");
         exit(1);
     }
 
@@ -2560,8 +2561,8 @@  static int sclp_parse(const char *devname)
     snprintf(label, sizeof(label), "sclpcon%d", index);
     sclp_hds[index] = qemu_chr_new(label, devname, NULL);
     if (!sclp_hds[index]) {
-        fprintf(stderr, "qemu: could not connect sclp console"
-                " to character backend '%s'\n", devname);
+        error_report("could not connect sclp console"
+                     " to character backend '%s'", devname);
         return -1;
     }
     qemu_opt_set(dev_opts, "chardev", label, &error_abort);
@@ -2579,7 +2580,7 @@  static int debugcon_parse(const char *devname)
     }
     opts = qemu_opts_create(qemu_find_opts("device"), "debugcon", 1, NULL);
     if (!opts) {
-        fprintf(stderr, "qemu: already have a debugcon device\n");
+        error_report("already have a debugcon device");
         exit(1);
     }
     qemu_opt_set(opts, "driver", "isa-debugcon", &error_abort);
@@ -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);
     }
     rtc_clock = QEMU_CLOCK_HOST;
@@ -3164,7 +3164,7 @@  int main(int argc, char **argv, char **envp)
                         }
                     } else if (*p != '\0') {
                     chs_fail:
-                        fprintf(stderr, "qemu: invalid physical CHS format\n");
+                        error_report("invalid physical CHS format");
                         exit(1);
                     }
                     if (hda_opts != NULL) {
@@ -3207,7 +3207,7 @@  int main(int argc, char **argv, char **envp)
 #ifdef CONFIG_CURSES
                 display_type = DT_CURSES;
 #else
-                fprintf(stderr, "Curses support is disabled\n");
+                error_report("Curses support is disabled");
                 exit(1);
 #endif
                 break;
@@ -3218,8 +3218,7 @@  int main(int argc, char **argv, char **envp)
                 graphic_rotate = strtol(optarg, (char **) &optarg, 10);
                 if (graphic_rotate != 0 && graphic_rotate != 90 &&
                     graphic_rotate != 180 && graphic_rotate != 270) {
-                    fprintf(stderr,
-                        "qemu: only 90, 180, 270 deg rotation is available\n");
+                    error_report("only 90, 180, 270 deg rotation is available");
                     exit(1);
                 }
                 break;
@@ -3370,7 +3369,7 @@  int main(int argc, char **argv, char **envp)
                     w = strtol(p, (char **)&p, 10);
                     if (w <= 0) {
                     graphic_error:
-                        fprintf(stderr, "qemu: invalid resolution or depth\n");
+                        error_report("invalid resolution or depth");
                         exit(1);
                     }
                     if (*p != 'x')
@@ -3436,7 +3435,7 @@  int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_fsdev:
                 olist = qemu_find_opts("fsdev");
                 if (!olist) {
-                    fprintf(stderr, "fsdev is not supported by this qemu build.\n");
+                    error_report("fsdev is not supported by this qemu build.");
                     exit(1);
                 }
                 opts = qemu_opts_parse_noisily(olist, optarg, true);
@@ -3451,7 +3450,7 @@  int main(int argc, char **argv, char **envp)
 
                 olist = qemu_find_opts("virtfs");
                 if (!olist) {
-                    fprintf(stderr, "virtfs is not supported by this qemu build.\n");
+                    error_report("virtfs is not supported by this qemu build.");
                     exit(1);
                 }
                 opts = qemu_opts_parse_noisily(olist, optarg, true);
@@ -3461,15 +3460,15 @@  int main(int argc, char **argv, char **envp)
 
                 if (qemu_opt_get(opts, "fsdriver") == NULL ||
                     qemu_opt_get(opts, "mount_tag") == NULL) {
-                    fprintf(stderr, "Usage: -virtfs fsdriver,mount_tag=tag.\n");
+                    error_report("Usage: -virtfs fsdriver,mount_tag=tag.");
                     exit(1);
                 }
                 fsdev = qemu_opts_create(qemu_find_opts("fsdev"),
                                          qemu_opt_get(opts, "mount_tag"),
                                          1, NULL);
                 if (!fsdev) {
-                    fprintf(stderr, "duplicate fsdev id: %s\n",
-                            qemu_opt_get(opts, "mount_tag"));
+                    error_report("duplicate fsdev id: %s",
+                                 qemu_opt_get(opts, "mount_tag"));
                     exit(1);
                 }
 
@@ -3478,8 +3477,7 @@  int main(int argc, char **argv, char **envp)
 #ifdef CONFIG_SYNC_FILE_RANGE
                     qemu_opt_set(fsdev, "writeout", writeout, &error_abort);
 #else
-                    fprintf(stderr, "writeout=immediate not supported on "
-                            "this platform\n");
+                    error_report("writeout=immediate not supported on this platform");
                     exit(1);
 #endif
                 }
@@ -3518,7 +3516,7 @@  int main(int argc, char **argv, char **envp)
                 fsdev = qemu_opts_create(qemu_find_opts("fsdev"), "v_synth",
                                          1, NULL);
                 if (!fsdev) {
-                    fprintf(stderr, "duplicate option: %s\n", "virtfs_synth");
+                    error_report("duplicate option: %s", "virtfs_synth");
                     exit(1);
                 }
                 qemu_opt_set(fsdev, "fsdriver", "synth", &error_abort);
@@ -3539,15 +3537,14 @@  int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_watchdog:
                 if (watchdog) {
-                    fprintf(stderr,
-                            "qemu: only one watchdog option may be given\n");
+                    error_report("only one watchdog option may be given");
                     return 1;
                 }
                 watchdog = optarg;
                 break;
             case QEMU_OPTION_watchdog_action:
                 if (select_watchdog_action(optarg) == -1) {
-                    fprintf(stderr, "Unknown -watchdog-action parameter\n");
+                    error_report("Unknown -watchdog-action parameter");
                     exit(1);
                 }
                 break;
@@ -3591,7 +3588,7 @@  int main(int argc, char **argv, char **envp)
                 display_type = DT_SDL;
                 break;
 #else
-                fprintf(stderr, "SDL support is disabled\n");
+                error_report("SDL support is disabled");
                 exit(1);
 #endif
             case QEMU_OPTION_pidfile:
@@ -3653,8 +3650,8 @@  int main(int argc, char **argv, char **envp)
                 qemu_opts_parse_noisily(olist, "accel=tcg", false);
                 break;
             case QEMU_OPTION_no_kvm_pit: {
-                fprintf(stderr, "Warning: KVM PIT can no longer be disabled "
-                                "separately.\n");
+                error_report("Warning: KVM PIT can no longer be disabled "
+                             "separately.");
                 break;
             }
             case QEMU_OPTION_no_kvm_pit_reinjection: {
@@ -3667,8 +3664,8 @@  int main(int argc, char **argv, char **envp)
                     { /* end of list */ }
                 };
 
-                fprintf(stderr, "Warning: option deprecated, use "
-                        "lost_tick_policy property of kvm-pit instead.\n");
+                error_report("Warning: option deprecated, use "
+                             "lost_tick_policy property of kvm-pit instead.");
                 qdev_prop_register_global_list(kvm_pit_lost_tick_policy);
                 break;
             }
@@ -3703,7 +3700,7 @@  int main(int argc, char **argv, char **envp)
                     exit(1);
                 }
 #else
-                fprintf(stderr, "VNC support is disabled\n");
+                error_report("VNC support is disabled");
                 exit(1);
 #endif
                 break;
@@ -3716,7 +3713,7 @@  int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_balloon:
                 if (balloon_parse(optarg) < 0) {
-                    fprintf(stderr, "Unknown -balloon argument %s\n", optarg);
+                    error_report("Unknown -balloon argument %s", optarg);
                     exit(1);
                 }
                 break;
@@ -3731,15 +3728,15 @@  int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_uuid:
                 if(qemu_uuid_parse(optarg, qemu_uuid) < 0) {
-                    fprintf(stderr, "Fail to parse UUID string."
-                            " Wrong format.\n");
+                    error_report("Fail to parse UUID string."
+                                 " Wrong format.");
                     exit(1);
                 }
                 qemu_uuid_set = true;
                 break;
             case QEMU_OPTION_option_rom:
                 if (nb_option_roms >= MAX_OPTION_ROMS) {
-                    fprintf(stderr, "Too many option ROMs\n");
+                    error_report("Too many option ROMs");
                     exit(1);
                 }
                 opts = qemu_opts_parse_noisily(qemu_find_opts("option-rom"),
@@ -3751,7 +3748,7 @@  int main(int argc, char **argv, char **envp)
                 option_rom[nb_option_roms].bootindex =
                     qemu_opt_get_number(opts, "bootindex", -1);
                 if (!option_rom[nb_option_roms].name) {
-                    fprintf(stderr, "Option ROM file is not specified\n");
+                    error_report("Option ROM file is not specified");
                     exit(1);
                 }
                 nb_option_roms++;
@@ -3776,9 +3773,8 @@  int main(int argc, char **argv, char **envp)
                         } else  if (strcmp("auto", target) == 0) {
                             semihosting.target = SEMIHOSTING_TARGET_AUTO;
                         } else {
-                            fprintf(stderr, "Unsupported semihosting-config"
-                                    " %s\n",
-                                optarg);
+                            error_report("Unsupported semihosting-config %s",
+                                         optarg);
                             exit(1);
                         }
                     } else {
@@ -3788,14 +3784,14 @@  int main(int argc, char **argv, char **envp)
                     qemu_opt_foreach(opts, add_semihosting_arg,
                                      &semihosting, NULL);
                 } else {
-                    fprintf(stderr, "Unsupported semihosting-config %s\n",
-                            optarg);
+                    error_report("Unsupported semihosting-config %s",
+                                 optarg);
                     exit(1);
                 }
                 break;
             case QEMU_OPTION_tdf:
-                fprintf(stderr, "Warning: user space PIT time drift fix "
-                                "is no longer supported.\n");
+                error_report("Warning: user space PIT time drift fix "
+                             "is no longer supported.");
                 break;
             case QEMU_OPTION_name:
                 opts = qemu_opts_parse_noisily(qemu_find_opts("name"),
@@ -3806,7 +3802,7 @@  int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_prom_env:
                 if (nb_prom_envs >= MAX_PROM_ENVS) {
-                    fprintf(stderr, "Too many prom variables\n");
+                    error_report("Too many prom variables");
                     exit(1);
                 }
                 prom_envs[nb_prom_envs] = optarg;
@@ -3889,8 +3885,8 @@  int main(int argc, char **argv, char **envp)
                 {
                     int ret = qemu_read_config_file(optarg);
                     if (ret < 0) {
-                        fprintf(stderr, "read config %s: %s\n", optarg,
-                            strerror(-ret));
+                        error_report("read config %s: %s", optarg,
+                                     strerror(-ret));
                         exit(1);
                     }
                     break;
@@ -3898,7 +3894,7 @@  int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_spice:
                 olist = qemu_find_opts("spice");
                 if (!olist) {
-                    fprintf(stderr, "spice is not supported by this qemu build.\n");
+                    error_report("spice is not supported by this qemu build.");
                     exit(1);
                 }
                 opts = qemu_opts_parse_noisily(olist, optarg, false);
@@ -3915,7 +3911,7 @@  int main(int argc, char **argv, char **envp)
                     } else {
                         fp = fopen(optarg, "w");
                         if (fp == NULL) {
-                            fprintf(stderr, "open %s: %s\n", optarg, strerror(errno));
+                            error_report("open %s: %s", optarg, strerror(errno));
                             exit(1);
                         }
                     }
@@ -3976,13 +3972,13 @@  int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_dump_vmstate:
                 if (vmstate_dump_file) {
-                    fprintf(stderr, "qemu: only one '-dump-vmstate' "
-                            "option may be given\n");
+                    error_report("only one '-dump-vmstate' "
+                                 "option may be given");
                     exit(1);
                 }
                 vmstate_dump_file = fopen(optarg, "w");
                 if (vmstate_dump_file == NULL) {
-                    fprintf(stderr, "open %s: %s\n", optarg, strerror(errno));
+                    error_report("open %s: %s", optarg, strerror(errno));
                     exit(1);
                 }
                 break;
@@ -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!");
         exit(1);
     }
 
@@ -4101,9 +4097,9 @@  int main(int argc, char **argv, char **envp)
 
     machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */
     if (max_cpus > machine_class->max_cpus) {
-        fprintf(stderr, "Number of SMP CPUs requested (%d) exceeds max CPUs "
-                "supported by machine '%s' (%d)\n", max_cpus,
-                machine_class->name, machine_class->max_cpus);
+        error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
+                     "supported by machine '%s' (%d)", max_cpus,
+                     machine_class->name, machine_class->max_cpus);
         exit(1);
     }
 
@@ -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");
             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");
             exit(1);
         }
 #endif
@@ -4228,12 +4224,12 @@  int main(int argc, char **argv, char **envp)
     }
 
     if ((no_frame || alt_grab || ctrl_grab) && display_type != DT_SDL) {
-        fprintf(stderr, "-no-frame, -alt-grab and -ctrl-grab are only valid "
-                        "for SDL, ignoring option\n");
+        error_report("-no-frame, -alt-grab and -ctrl-grab are only valid "
+                     "for SDL, ignoring option");
     }
     if (no_quit && (display_type != DT_GTK && display_type != DT_SDL)) {
-        fprintf(stderr, "-no-quit is only valid for GTK and SDL, "
-                        "ignoring option\n");
+        error_report("-no-quit is only valid for GTK and SDL, "
+                     "ignoring option");
     }
 
 #if defined(CONFIG_GTK)
@@ -4248,9 +4244,9 @@  int main(int argc, char **argv, char **envp)
 #endif
     if (request_opengl == 1 && display_opengl == 0) {
 #if defined(CONFIG_OPENGL)
-        fprintf(stderr, "OpenGL is not supported by the display.\n");
+        error_report("OpenGL is not supported by the display.");
 #else
-        fprintf(stderr, "QEMU was built without opengl support.\n");
+        error_report("QEMU was built without opengl support.");
 #endif
         exit(1);
     }
@@ -4276,7 +4272,7 @@  int main(int argc, char **argv, char **envp)
 #endif
 
     if (pid_file && qemu_create_pidfile(pid_file) != 0) {
-        fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
+        error_report("Could not acquire pid file: %s", strerror(errno));
         exit(1);
     }
 
@@ -4347,17 +4343,17 @@  int main(int argc, char **argv, char **envp)
     linux_boot = (kernel_filename != NULL);
 
     if (!linux_boot && *kernel_cmdline != '\0') {
-        fprintf(stderr, "-append only allowed with -kernel option\n");
+        error_report("-append only allowed with -kernel option");
         exit(1);
     }
 
     if (!linux_boot && initrd_filename != NULL) {
-        fprintf(stderr, "-initrd only allowed with -kernel option\n");
+        error_report("-initrd only allowed with -kernel option");
         exit(1);
     }
 
     if (!linux_boot && qemu_opt_get(machine_opts, "dtb")) {
-        fprintf(stderr, "-dtb only allowed with -kernel option\n");
+        error_report("-dtb only allowed with -kernel option");
         exit(1);
     }
 
@@ -4376,7 +4372,7 @@  int main(int argc, char **argv, char **envp)
     cpu_ticks_init();
     if (icount_opts) {
         if (kvm_enabled() || xen_enabled()) {
-            fprintf(stderr, "-icount is not allowed with kvm or xen\n");
+            error_report("-icount is not allowed with kvm or xen");
             exit(1);
         }
         configure_icount(icount_opts, &error_abort);
@@ -4409,7 +4405,7 @@  int main(int argc, char **argv, char **envp)
     if (!xen_enabled()) {
         /* On 32-bit hosts, QEMU is limited by virtual address space */
         if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
-            fprintf(stderr, "qemu: at most 2047 MB RAM can be simulated\n");
+            error_report("at most 2047 MB RAM can be simulated");
             exit(1);
         }
     }
@@ -4596,7 +4592,7 @@  int main(int argc, char **argv, char **envp)
     qemu_run_machine_init_done_notifiers();
 
     if (rom_check_and_register_reset() != 0) {
-        fprintf(stderr, "rom check and register reset failed\n");
+        error_report("rom check and register reset failed");
         exit(1);
     }