Re: [Qemu-devel] [PATCH 1/2] Mostly revert qemu-help: Sort devices by logical functionality

2013-10-11 Thread Markus Armbruster 
Marcel Apfelbaum marce...@redhat.com writes:

 On Thu, 2013-10-10 at 15:00 +0200, arm...@redhat.com wrote:
 From: Markus Armbruster arm...@redhat.com
 
 This reverts most of commit 3d1237fb2ab4edb926c717767bb5e31d6053a7c5.
 
 The commit claims to sort the output of -device help by
 functionality rather than alphabetical.  Issues:
 
 * The output was unsorted before, not alphabetically sorted.
   Misleading, but harmless enough.
 I meant that will be sorted, but the sorting will be by functionality
 rather than alphabetical. I can understand the confusion.

Happens :)

 * The commit doesn't just sort the output of -device help as it
   claims, it adds categories to each line of -device help, and it
   prints devices once per category.  In particular, devices without a
   category aren't shown anymore.  Maybe such devices should not exist,
   but they do.  Regression.
 Yes - :(, it was a drawback that devices with no category are not
 printed. On the other hand all devices must be attached to a category,
 otherwise we will have again a lot of devices with no category.

s/otherweise will have again/but unfortunately we still have/

 I suppose your approach lets us at least with the possibility to see
 these devices giving us a chance to attach them to their category.

Yes.

If you want to enforce no uncategorized devices, you need to catch
them reliably at compile time or make check time.  Once that's done,
we can simplify my qdev_print_class_devinfos() not to look for such
devices.  Patches welcome :)

 * Categories are also added to the output of info qdm.  Silent
   change, not nice.  Output remains unsorted, unlike -device help.

 I checked libvirt and the parsing of the output is not affected
 by adding the category. 

I didn't claim actual harm, just pointed out that you should've
mentioned it in the commit message, and that the reasons for improving
device_add help equally apply to info qdm.

 I still think that adding the category in each line may be
 useful when using grep, but I suppose that we can grep by category 
 with -A x.

Yes, repeating the categories on each line can be convenient when the
lines are fed to programs, but help output is primarily for humans.

Try 'sed -n /^$cat devices:/,/^\$/p' instead of 'grep $cat'.

[...]

[Qemu-devel] [PATCH 1/2] Mostly revert qemu-help: Sort devices by logical functionality

2013-10-10 Thread armbru 
From: Markus Armbruster arm...@redhat.com

This reverts most of commit 3d1237fb2ab4edb926c717767bb5e31d6053a7c5.

The commit claims to sort the output of -device help by
functionality rather than alphabetical.  Issues:

* The output was unsorted before, not alphabetically sorted.
  Misleading, but harmless enough.

* The commit doesn't just sort the output of -device help as it
  claims, it adds categories to each line of -device help, and it
  prints devices once per category.  In particular, devices without a
  category aren't shown anymore.  Maybe such devices should not exist,
  but they do.  Regression.

* Categories are also added to the output of info qdm.  Silent
  change, not nice.  Output remains unsorted, unlike -device help.

I'm going to reimplement the feature we actually want, without the
warts.  Reverting the flawed commit first should make it easier to
review.  However, I can't revert it completely, since DeviceClass
member categories has been put to use.  So leave that part in.

Signed-off-by: Markus Armbruster arm...@redhat.com
---
 include/hw/qdev-core.h | 16 
 qdev-monitor.c | 48 +---
 2 files changed, 9 insertions(+), 55 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index a62f231..e191ca0 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -30,22 +30,6 @@ typedef enum DeviceCategory {
 DEVICE_CATEGORY_MAX
 } DeviceCategory;
 
-static inline const char *qdev_category_get_name(DeviceCategory category)
-{
-static const char *category_names[DEVICE_CATEGORY_MAX] = {
-[DEVICE_CATEGORY_BRIDGE]  = Controller/Bridge/Hub,
-[DEVICE_CATEGORY_USB] = USB,
-[DEVICE_CATEGORY_STORAGE] = Storage,
-[DEVICE_CATEGORY_NETWORK] = Network,
-[DEVICE_CATEGORY_INPUT]   = Input,
-[DEVICE_CATEGORY_DISPLAY] = Display,
-[DEVICE_CATEGORY_SOUND]   = Sound,
-[DEVICE_CATEGORY_MISC]= Misc,
-};
-
-return category_names[category];
-};
-
 typedef int (*qdev_initfn)(DeviceState *dev);
 typedef int (*qdev_event)(DeviceState *dev);
 typedef void (*qdev_resetfn)(DeviceState *dev);
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 410cdcb..e5adf6c 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -75,27 +75,24 @@ static bool qdev_class_has_alias(DeviceClass *dc)
 return (qdev_class_get_alias(dc) != NULL);
 }
 
-static void qdev_print_class_devinfo(DeviceClass *dc)
+static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
 {
-DeviceCategory category;
+DeviceClass *dc;
+bool *show_no_user = opaque;
+
+dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE);
 
-if (!dc) {
+if (!dc || (show_no_user  !*show_no_user  dc-no_user)) {
 return;
 }
 
-error_printf(name \%s\, object_class_get_name(OBJECT_CLASS(dc)));
+error_printf(name \%s\, object_class_get_name(klass));
 if (dc-bus_type) {
 error_printf(, bus %s, dc-bus_type);
 }
 if (qdev_class_has_alias(dc)) {
 error_printf(, alias \%s\, qdev_class_get_alias(dc));
 }
-error_printf(, categories);
-for (category = 0; category  DEVICE_CATEGORY_MAX; ++category) {
-if (test_bit(category, dc-categories)) {
-error_printf( \%s\, qdev_category_get_name(category));
-}
-}
 if (dc-desc) {
 error_printf(, desc \%s\, dc-desc);
 }
@@ -105,15 +102,6 @@ static void qdev_print_class_devinfo(DeviceClass *dc)
 error_printf(\n);
 }
 
-static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
-{
-DeviceClass *dc;
-
-dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE);
-
-qdev_print_class_devinfo(dc);
-}
-
 static int set_property(const char *name, const char *value, void *opaque)
 {
 DeviceState *dev = opaque;
@@ -151,21 +139,6 @@ static const char *find_typename_by_alias(const char 
*alias)
 return NULL;
 }
 
-static void qdev_print_category_devices(DeviceCategory category)
-{
-DeviceClass *dc;
-GSList *list, *curr;
-
-list = object_class_get_list(TYPE_DEVICE, false);
-for (curr = list; curr; curr = g_slist_next(curr)) {
-dc = (DeviceClass *)object_class_dynamic_cast(curr-data, TYPE_DEVICE);
-if (!dc-no_user  test_bit(category, dc-categories)) {
-qdev_print_class_devinfo(dc);
-}
-}
-g_slist_free(list);
-}
-
 int qdev_device_help(QemuOpts *opts)
 {
 const char *driver;
@@ -174,11 +147,8 @@ int qdev_device_help(QemuOpts *opts)
 
 driver = qemu_opt_get(opts, driver);
 if (driver  is_help_option(driver)) {
-DeviceCategory category;
-for (category = 0; category  DEVICE_CATEGORY_MAX; ++category) {
-qdev_print_category_devices(category);
-}
-
+bool show_no_user = false;
+object_class_foreach(qdev_print_devinfo, TYPE_DEVICE, false, 
show_no_user);
 return 1;
 }
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/2] Mostly revert qemu-help: Sort devices by logical functionality

2013-10-10 Thread Marcel Apfelbaum 
On Thu, 2013-10-10 at 15:00 +0200, arm...@redhat.com wrote:
 From: Markus Armbruster arm...@redhat.com
 
 This reverts most of commit 3d1237fb2ab4edb926c717767bb5e31d6053a7c5.
 
 The commit claims to sort the output of -device help by
 functionality rather than alphabetical.  Issues:
 
 * The output was unsorted before, not alphabetically sorted.
   Misleading, but harmless enough.
I meant that will be sorted, but the sorting will be by functionality
rather than alphabetical. I can understand the confusion.

 
 * The commit doesn't just sort the output of -device help as it
   claims, it adds categories to each line of -device help, and it
   prints devices once per category.  In particular, devices without a
   category aren't shown anymore.  Maybe such devices should not exist,
   but they do.  Regression.
Yes - :(, it was a drawback that devices with no category are not
printed. On the other hand all devices must be attached to a category,
otherwise we will have again a lot of devices with no category.

I suppose your approach lets us at least with the possibility to see
these devices giving us a chance to attach them to their category.

 
 * Categories are also added to the output of info qdm.  Silent
   change, not nice.  Output remains unsorted, unlike -device help.

I checked libvirt and the parsing of the output is not affected
by adding the category. 

I still think that adding the category in each line may be
useful when using grep, but I suppose that we can grep by category 
with -A x.

Thanks,
Marcel 

 I'm going to reimplement the feature we actually want, without the
 warts.  Reverting the flawed commit first should make it easier to
 review.  However, I can't revert it completely, since DeviceClass
 member categories has been put to use.  So leave that part in.
 
 Signed-off-by: Markus Armbruster arm...@redhat.com
 ---
  include/hw/qdev-core.h | 16 
  qdev-monitor.c | 48 +---
  2 files changed, 9 insertions(+), 55 deletions(-)
 
 diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
 index a62f231..e191ca0 100644
 --- a/include/hw/qdev-core.h
 +++ b/include/hw/qdev-core.h
 @@ -30,22 +30,6 @@ typedef enum DeviceCategory {
  DEVICE_CATEGORY_MAX
  } DeviceCategory;
  
 -static inline const char *qdev_category_get_name(DeviceCategory category)
 -{
 -static const char *category_names[DEVICE_CATEGORY_MAX] = {
 -[DEVICE_CATEGORY_BRIDGE]  = Controller/Bridge/Hub,
 -[DEVICE_CATEGORY_USB] = USB,
 -[DEVICE_CATEGORY_STORAGE] = Storage,
 -[DEVICE_CATEGORY_NETWORK] = Network,
 -[DEVICE_CATEGORY_INPUT]   = Input,
 -[DEVICE_CATEGORY_DISPLAY] = Display,
 -[DEVICE_CATEGORY_SOUND]   = Sound,
 -[DEVICE_CATEGORY_MISC]= Misc,
 -};
 -
 -return category_names[category];
 -};
 -
  typedef int (*qdev_initfn)(DeviceState *dev);
  typedef int (*qdev_event)(DeviceState *dev);
  typedef void (*qdev_resetfn)(DeviceState *dev);
 diff --git a/qdev-monitor.c b/qdev-monitor.c
 index 410cdcb..e5adf6c 100644
 --- a/qdev-monitor.c
 +++ b/qdev-monitor.c
 @@ -75,27 +75,24 @@ static bool qdev_class_has_alias(DeviceClass *dc)
  return (qdev_class_get_alias(dc) != NULL);
  }
  
 -static void qdev_print_class_devinfo(DeviceClass *dc)
 +static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
  {
 -DeviceCategory category;
 +DeviceClass *dc;
 +bool *show_no_user = opaque;
 +
 +dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE);
  
 -if (!dc) {
 +if (!dc || (show_no_user  !*show_no_user  dc-no_user)) {
  return;
  }
  
 -error_printf(name \%s\, object_class_get_name(OBJECT_CLASS(dc)));
 +error_printf(name \%s\, object_class_get_name(klass));
  if (dc-bus_type) {
  error_printf(, bus %s, dc-bus_type);
  }
  if (qdev_class_has_alias(dc)) {
  error_printf(, alias \%s\, qdev_class_get_alias(dc));
  }
 -error_printf(, categories);
 -for (category = 0; category  DEVICE_CATEGORY_MAX; ++category) {
 -if (test_bit(category, dc-categories)) {
 -error_printf( \%s\, qdev_category_get_name(category));
 -}
 -}
  if (dc-desc) {
  error_printf(, desc \%s\, dc-desc);
  }
 @@ -105,15 +102,6 @@ static void qdev_print_class_devinfo(DeviceClass *dc)
  error_printf(\n);
  }
  
 -static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
 -{
 -DeviceClass *dc;
 -
 -dc = (DeviceClass *)object_class_dynamic_cast(klass, TYPE_DEVICE);
 -
 -qdev_print_class_devinfo(dc);
 -}
 -
  static int set_property(const char *name, const char *value, void *opaque)
  {
  DeviceState *dev = opaque;
 @@ -151,21 +139,6 @@ static const char *find_typename_by_alias(const char 
 *alias)
  return NULL;
  }
  
 -static void qdev_print_category_devices(DeviceCategory category)
 -{
 -DeviceClass *dc;
 -GSList *list, *curr;
 -
 -