Patchwork [RFC,15/48] error: Don't abuse qemu_error() for non-error in qdev_device_help()

login
register
mail settings
Submitter Markus Armbruster
Date Feb. 24, 2010, 5:55 p.m.
Message ID <1267034160-3517-16-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/46180/
State New
Headers show

Comments

Markus Armbruster - Feb. 24, 2010, 5:55 p.m.
qdev_device_help() prints device information with qemu_error().  A
later commit will make qemu_error() print additional stuff that is
only appropriate for proper errors, and then this will break.  Use
error_printf() instead.

While there, simplify: instead of printing a buffer filled by
qdev_print_devinfo() in one go, make qdev_print_devinfo() print it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/qdev.c |   31 ++++++++++---------------------
 1 files changed, 10 insertions(+), 21 deletions(-)
Luiz Capitulino - Feb. 26, 2010, 7:44 p.m.
On Wed, 24 Feb 2010 18:55:27 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> qdev_device_help() prints device information with qemu_error().  A
> later commit will make qemu_error() print additional stuff that is
> only appropriate for proper errors, and then this will break.  Use
> error_printf() instead.

 Aren't you abusing the error function just like the current code? Also,
I think that this information should be printed to stdout.
Markus Armbruster - March 1, 2010, 9:05 a.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 24 Feb 2010 18:55:27 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> qdev_device_help() prints device information with qemu_error().  A
>> later commit will make qemu_error() print additional stuff that is
>> only appropriate for proper errors, and then this will break.  Use
>> error_printf() instead.
>
>  Aren't you abusing the error function just like the current code?

error_printf() isn't a function to report an error.  It's a function to
print to the error destination, which is either the current monitor or
stderr.  It serves as a building block for functions that report an
error.

>                                                                    Also,
> I think that this information should be printed to stdout.

Agree in principle.  Existing code is sloppy about stdout vs. stderr,
however.  If it's important to start cleaning that up *now*, we could
add functions to print to the "user destination", which is either the
current monitor or stdout.

In any case, it needs to be a separate commit, because this commit
should do just one thing: fix the qemu_error() abuse.  Proper use of
stderr and stdout is a separate issue.
Luiz Capitulino - March 1, 2010, 4:11 p.m.
On Mon, 01 Mar 2010 10:05:18 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 24 Feb 2010 18:55:27 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> qdev_device_help() prints device information with qemu_error().  A
> >> later commit will make qemu_error() print additional stuff that is
> >> only appropriate for proper errors, and then this will break.  Use
> >> error_printf() instead.
> >
> >  Aren't you abusing the error function just like the current code?
> 
> error_printf() isn't a function to report an error.  It's a function to
> print to the error destination, which is either the current monitor or
> stderr.  It serves as a building block for functions that report an
> error.

 Makes sense.

> > I think that this information should be printed to stdout.
> 
> Agree in principle.  Existing code is sloppy about stdout vs. stderr,
> however.  If it's important to start cleaning that up *now*, we could
> add functions to print to the "user destination", which is either the
> current monitor or stdout.
> 
> In any case, it needs to be a separate commit, because this commit
> should do just one thing: fix the qemu_error() abuse.  Proper use of
> stderr and stdout is a separate issue.

 Okay.

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index a992479..377e327 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -113,27 +113,20 @@  DeviceState *qdev_create(BusState *bus, const char *name)
     return dev;
 }
 
-static int qdev_print_devinfo(DeviceInfo *info, char *dest, int len)
+static void qdev_print_devinfo(DeviceInfo *info)
 {
-    int pos = 0;
-    int ret;
-
-    ret = snprintf(dest+pos, len-pos, "name \"%s\", bus %s",
-                   info->name, info->bus_info->name);
-    pos += MIN(len-pos,ret);
+    error_printf("name \"%s\", bus %s",
+                 info->name, info->bus_info->name);
     if (info->alias) {
-        ret = snprintf(dest+pos, len-pos, ", alias \"%s\"", info->alias);
-        pos += MIN(len-pos,ret);
+        error_printf(", alias \"%s\"", info->alias);
     }
     if (info->desc) {
-        ret = snprintf(dest+pos, len-pos, ", desc \"%s\"", info->desc);
-        pos += MIN(len-pos,ret);
+        error_printf(", desc \"%s\"", info->desc);
     }
     if (info->no_user) {
-        ret = snprintf(dest+pos, len-pos, ", no-user");
-        pos += MIN(len-pos,ret);
+        error_printf(", no-user");
     }
-    return pos;
+    error_printf("\n");
 }
 
 static int set_property(const char *name, const char *value, void *opaque)
@@ -157,14 +150,12 @@  int qdev_device_help(QemuOpts *opts)
 {
     const char *driver;
     DeviceInfo *info;
-    char msg[256];
     Property *prop;
 
     driver = qemu_opt_get(opts, "driver");
     if (driver && !strcmp(driver, "?")) {
         for (info = device_info_list; info != NULL; info = info->next) {
-            qdev_print_devinfo(info, msg, sizeof(msg));
-            qemu_error("%s", msg);
+            qdev_print_devinfo(info);
         }
         return 1;
     }
@@ -179,7 +170,7 @@  int qdev_device_help(QemuOpts *opts)
     }
 
     for (prop = info->props; prop && prop->name; prop++) {
-        qemu_error("%s.%s=%s", info->name, prop->name, prop->info->name);
+        error_printf("%s.%s=%s\n", info->name, prop->name, prop->info->name);
     }
     return 1;
 }
@@ -735,11 +726,9 @@  void do_info_qtree(Monitor *mon)
 void do_info_qdm(Monitor *mon)
 {
     DeviceInfo *info;
-    char msg[256];
 
     for (info = device_info_list; info != NULL; info = info->next) {
-        qdev_print_devinfo(info, msg, sizeof(msg));
-        monitor_printf(mon, "%s\n", msg);
+        qdev_print_devinfo(info);
     }
 }