modinfo: rework parm handling

Currently, we grow a list of parm entries and print each entry upon
being added. In addition, we have two separate code-paths for printing
depending if "--field parm" was provided on not.

Swap that for sufficiently sized, pre-allocated, buffers and unify the
print paths. Effectively fixing the broken `modinfo -0 modulename`
output.

v2:
 - Find matching or empty entry in a single loop (add_param)
 - match up-to UINT_MAX parm/parmtype entries
 - drop unneeded INT_MAX checks for strlen(value)
 - don't return success if we fail to [cm]alloc

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Link: https://github.com/kmod-project/kmod/pull/451
Signed-off-by: Lucas De Marchi <demarchi@kernel.org>

diff --git a/tools/modinfo.c b/tools/modinfo.c
index b84850151ee5b82c5f94862ced1ae0ad22be4863..1ccdfaaf871811109fa9a8ff4410545a200ad989 100644 (file)
--- a/tools/modinfo.c
+++ b/tools/modinfo.c
@@ -26,7 +26,6 @@ static char separator = '\n';
 static const char *field;
 
 struct param {
-       struct param *next;
        const char *name;
        const char *desc;
        const char *type;
@@ -41,7 +40,7 @@ enum parm_info {
 };
 
 static int add_param(const char *name, size_t namelen, enum parm_info parm_info,
-                    const char *value, struct param **list)
+                    const char *value, struct param *params, unsigned int params_count)
 {
        size_t valuelen = strlen(value);
        struct param *it;
@@ -49,40 +48,36 @@ static int add_param(const char *name, size_t namelen, enum parm_info parm_info,
        if (namelen > INT_MAX || valuelen > INT_MAX)
                return -EINVAL;
 
-       for (it = *list; it != NULL; it = it->next) {
-               if (it->namelen == (int)namelen && memcmp(it->name, name, namelen) == 0)
-                       break;
-       }
+       /* We are guaranteed to have a match, or at least one empty entry */
+       for (unsigned int i = 0; i < params_count; i++) {
+               it = &params[i];
+
+               if (it->name != NULL && (it->namelen != (int)namelen ||
+                                        memcmp(it->name, name, namelen) != 0)) {
+                       continue;
+               }
 
-       if (it == NULL) {
-               it = malloc(sizeof(struct param));
-               if (it == NULL)
-                       return -ENOMEM;
-               it->next = *list;
-               *list = it;
                it->name = name;
                it->namelen = namelen;
-               it->desc = NULL;
-               it->type = NULL;
-               it->desclen = 0;
-               it->typelen = 0;
-       }
 
-       switch (parm_info) {
-       case (parm_desc):
-               it->desc = value;
-               it->desclen = (int)valuelen;
-               break;
-       case (parm_type):
-               it->type = value;
-               it->typelen = (int)valuelen;
+               switch (parm_info) {
+               case (parm_desc):
+                       it->desc = value;
+                       it->desclen = (int)valuelen;
+                       break;
+               case (parm_type):
+                       it->type = value;
+                       it->typelen = (int)valuelen;
+                       break;
+               }
                break;
        }
 
        return 0;
 }
 
-static int process_parm(enum parm_info parm_info, const char *value, struct param **params)
+static int process_parm(enum parm_info parm_info, const char *value, struct param *params,
+                       unsigned int params_count)
 {
        const char *name;
        size_t namelen;
@@ -101,7 +96,7 @@ static int process_parm(enum parm_info parm_info, const char *value, struct para
 
        name = value;
        namelen = colon - value;
-       ret = add_param(name, namelen, parm_info, colon + 1, params);
+       ret = add_param(name, namelen, parm_info, colon + 1, params, params_count);
        if (ret < 0) {
                ERR("Unable to add parameter: %s\n", strerror(-ret));
                return -ENOMEM;
@@ -127,50 +122,6 @@ static void print_line(const char *key, const char *value)
        }
 }
 
-static int modinfo_params_do(const struct kmod_list *list)
-{
-       const struct kmod_list *l;
-       struct param *params = NULL;
-       int err = 0;
-
-       kmod_list_foreach(l, list) {
-               const char *key = kmod_module_info_get_key(l);
-               const char *value = kmod_module_info_get_value(l);
-               if (streq(key, "parm")) {
-                       err = process_parm(parm_desc, value, &params);
-                       if (err < 0)
-                               goto end;
-               } else if (streq(key, "parmtype")) {
-                       err = process_parm(parm_type, value, &params);
-                       if (err < 0)
-                               goto end;
-               }
-       }
-
-       while (params != NULL) {
-               struct param *p = params;
-               params = p->next;
-
-               if (p->type != NULL)
-                       printf("%.*s:%.*s (%.*s)%c", p->namelen, p->name, p->desclen,
-                              p->desc, p->typelen, p->type, separator);
-               else
-                       printf("%.*s:%.*s%c", p->namelen, p->name, p->desclen, p->desc,
-                              separator);
-
-               free(p);
-       }
-
-end:
-       while (params != NULL) {
-               void *tmp = params;
-               params = params->next;
-               free(tmp);
-       }
-
-       return err;
-}
-
 static int modinfo_do(struct kmod_module *mod)
 {
        const enum kmod_module_initstate state = kmod_module_get_initstate(mod);
@@ -180,6 +131,8 @@ static int modinfo_do(struct kmod_module *mod)
        const bool print_parm = !print_all && streq(field, "parm");
        struct kmod_list *l, *list = NULL;
        struct param *params = NULL;
+       unsigned int params_count = 0;
+       char *sbuf = NULL;
        int err;
 
        /* TODO: align builtin vs not wrt listing "name:" via kmod_module_get_info() */
@@ -214,16 +167,58 @@ static int modinfo_do(struct kmod_module *mod)
                return err;
        }
 
-       if (print_parm) {
-               err = modinfo_params_do(list);
-               goto end;
+       if (print_all || print_parm) {
+               size_t count = 0;
+               size_t longest_desc = 0;
+               size_t longest_type = 0;
+
+               /*
+                * Usually parm/parmtype come in pairs, where we print once per pair.
+                *
+                * Get worst case scenario & longest entry, for sufficiently large buffers.
+                */
+               kmod_list_foreach(l, list) {
+                       const char *key = kmod_module_info_get_key(l);
+
+                       if (streq(key, "parm")) {
+                               const char *value = kmod_module_info_get_value(l);
+                               size_t len = strlen(value);
+
+                               count++;
+
+                               if (len > longest_desc)
+                                       longest_desc = len;
+                       } else if (streq(key, "parmtype")) {
+                               const char *value = kmod_module_info_get_value(l);
+                               size_t len = strlen(value);
+
+                               count++;
+
+                               if (len > longest_type)
+                                       longest_type = len;
+                       }
+               }
+
+               /* XXX: do we want to emit a warning/error here? */
+               params_count = (params_count > UINT_MAX) ? UINT_MAX : (unsigned int)count;
+
+               params = calloc(params_count, sizeof(*params));
+               /*
+                * The "name:" exists in both parm&parmtype, so don't worry if we
+                * overallocate.
+                */
+               sbuf = malloc(longest_desc + longest_type + strlen(" ()"));
+               if (params == NULL || sbuf == NULL) {
+                       err = -ENOMEM;
+                       goto end;
+               }
        }
 
        kmod_list_foreach(l, list) {
                const char *key = kmod_module_info_get_key(l);
                const char *value = kmod_module_info_get_value(l);
 
-               if (!print_all) {
+               if (!print_all && !print_parm) {
                        if (streq(field, key)) {
                                print_line(NULL, value);
                                goto end;
@@ -231,41 +226,37 @@ static int modinfo_do(struct kmod_module *mod)
                        continue;
                }
                if (streq(key, "parm")) {
-                       err = process_parm(parm_desc, value, &params);
+                       err = process_parm(parm_desc, value, params, params_count);
                        if (err < 0)
                                goto end;
                } else if (streq(key, "parmtype")) {
-                       err = process_parm(parm_type, value, &params);
+                       err = process_parm(parm_type, value, params, params_count);
                        if (err < 0)
                                goto end;
                } else {
-                       print_line(key, value);
+                       if (print_all)
+                               print_line(key, value);
                }
        }
 
-       if (!print_all)
-               goto end;
-
-       while (params != NULL) {
-               struct param *p = params;
-               params = p->next;
+       for (unsigned int i = 0; i < params_count; i++) {
+               struct param *p = &params[i];
+               if (p->name == NULL)
+                       continue;
 
                if (p->type != NULL)
-                       printf("%-16s%.*s:%.*s (%.*s)%c", "parm:", p->namelen, p->name,
-                              p->desclen, p->desc, p->typelen, p->type, separator);
+                       sprintf(sbuf, "%.*s:%.*s (%.*s)", p->namelen, p->name, p->desclen,
+                               p->desc, p->typelen, p->type);
                else
-                       printf("%-16s%.*s:%.*s%c", "parm:", p->namelen, p->name,
-                              p->desclen, p->desc, separator);
+                       sprintf(sbuf, "%.*s:%.*s", p->namelen, p->name, p->desclen,
+                               p->desc);
 
-               free(p);
+               print_line(print_parm ? NULL : "parm", sbuf);
        }
 
 end:
-       while (params != NULL) {
-               void *tmp = params;
-               params = params->next;
-               free(tmp);
-       }
+       free(sbuf);
+       free(params);
        kmod_module_info_free_list(list);
 
        return err;