From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56384) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VUHee-0002hg-94 for qemu-devel@nongnu.org; Thu, 10 Oct 2013 10:56:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VUHeY-0004cU-6i for qemu-devel@nongnu.org; Thu, 10 Oct 2013 10:55:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:6239) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VUHeX-0004cO-Uo for qemu-devel@nongnu.org; Thu, 10 Oct 2013 10:55:50 -0400 Message-ID: <1381416968.22271.18.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Thu, 10 Oct 2013 17:56:08 +0300 In-Reply-To: <1381410021-1538-2-git-send-email-armbru@redhat.com> References: <1381410021-1538-1-git-send-email-armbru@redhat.com> <1381410021-1538-2-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] Mostly revert "qemu-help: Sort devices by logical functionality" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: armbru@redhat.com Cc: qemu-devel@nongnu.org, anthony@codemonkey.ws On Thu, 2013-10-10 at 15:00 +0200, armbru@redhat.com wrote: > From: Markus Armbruster > > 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 > --- > 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; > } >