Re: [PATCH] util/qemu-config: Remove unnecessary traversal

[Markus Armbruster <armbru@redhat.com>]

Shiyuan Gao via <qemu-devel@nongnu.org> writes:

> From: Gao Shiyuan <gaoshiyuan@baidu.com>
>
> No remove QemuOptsList from *_config_groups, so no need to
> traverse from the beginning every time.
>
> No functional changes.
>
> Signed-off-by: Gao Shiyuan <gaoshiyuan@baidu.com>
> ---
>  include/qemu/config-file.h |  3 +++
>  util/qemu-config.c         | 18 ++++++++----------
>  2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
> index b82a778123..223ef7bc8f 100644
> --- a/include/qemu/config-file.h
> +++ b/include/qemu/config-file.h
> @@ -1,6 +1,9 @@
>  #ifndef QEMU_CONFIG_FILE_H
>  #define QEMU_CONFIG_FILE_H
>  
> +#define MAX_VM_CONFIG_GROUPS    48
> +#define MAX_DRIVE_CONFIG_GROUPS 5

These are not used outside qemu-config.c, so it's better to define them
there.

> +
>  typedef void QEMUConfigCB(const char *group, QDict *qdict, void *opaque, Error **errp);
>  
>  void qemu_load_module_for_opts(const char *group);
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 42076efe1e..d7bab2047f 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -9,8 +9,8 @@
>  #include "qemu/config-file.h"
>  #include "hw/boards.h"
>  
> -static QemuOptsList *vm_config_groups[48];
> -static QemuOptsList *drive_config_groups[5];
> +static QemuOptsList *vm_config_groups[MAX_VM_CONFIG_GROUPS];
> +static QemuOptsList *drive_config_groups[MAX_DRIVE_CONFIG_GROUPS];
>  
>  static QemuOptsList *find_list(QemuOptsList **lists, const char *group,
>                                 Error **errp)
> @@ -260,11 +260,10 @@ QemuOptsList *qemu_find_opts_err(const char *group, Error **errp)
>  
>  void qemu_add_drive_opts(QemuOptsList *list)
>  {
> -    int entries, i;
> +    static int i;
> +    static int entries = MAX_DRIVE_CONFIG_GROUPS - 1; /* keep list NULL terminated */
>  
> -    entries = ARRAY_SIZE(drive_config_groups);
> -    entries--; /* keep list NULL terminated */
> -    for (i = 0; i < entries; i++) {
> +    for (; i < entries; i++) {
>          if (drive_config_groups[i] == NULL) {
>              drive_config_groups[i] = list;
>              return;
> @@ -276,11 +275,10 @@ void qemu_add_drive_opts(QemuOptsList *list)
>  
>  void qemu_add_opts(QemuOptsList *list)
>  {
> -    int entries, i;
> +    static int i;
> +    static int entries = MAX_VM_CONFIG_GROUPS - 1; /* keep list NULL terminated */
>  
> -    entries = ARRAY_SIZE(vm_config_groups);
> -    entries--; /* keep list NULL terminated */
> -    for (i = 0; i < entries; i++) {
> +    for (; i < entries; i++) {
>          if (vm_config_groups[i] == NULL) {
>              vm_config_groups[i] = list;
>              return;

These two functions run at most 4 and 47 times, respectively.  I don't
think speeding them up matters.  Keeping them simple does.

Your patch conflates two separate changes.

1. Replace use of ARRAY_SIZE() by new macros.  Doesn't feel like an
improvement to me.

2. Optimize appending to arrays drive_config_groups[] and
vm_config_groups[].

Let's have a look at 2. without 1.:

diff --git a/util/qemu-config.c b/util/qemu-config.c
index 42076efe1e..7875359eb3 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -260,11 +260,13 @@ QemuOptsList *qemu_find_opts_err(const char *group, Error **errp)
 
 void qemu_add_drive_opts(QemuOptsList *list)
 {
-    int entries, i;
+    static int i;
+    int entries;
 
     entries = ARRAY_SIZE(drive_config_groups);
     entries--; /* keep list NULL terminated */
-    for (i = 0; i < entries; i++) {
+
+    for (; i < entries; i++) {
         if (drive_config_groups[i] == NULL) {
             drive_config_groups[i] = list;
             return;
@@ -276,11 +278,13 @@ void qemu_add_drive_opts(QemuOptsList *list)
 
 void qemu_add_opts(QemuOptsList *list)
 {
-    int entries, i;
+    static int i;
+    int entries;
 
     entries = ARRAY_SIZE(vm_config_groups);
     entries--; /* keep list NULL terminated */
-    for (i = 0; i < entries; i++) {
+
+    for (; i < entries; i++) {
         if (vm_config_groups[i] == NULL) {
             vm_config_groups[i] = list;
             return;

This is a more focused patch.

It makes the functions faster, which doesn't matter.  Does it make the
functions easier to understand?  That would matter.