Re: [Qemu-devel] [PATCH v9 5/8] module: implement module loading

[Richard Henderson]

On 09/13/2013 02:59 AM, Fam Zheng wrote:
> +    const char *module_whitelist[] = {
> +        CONFIG_MODULE_WHITELIST
> +    };

  static const char * const module_whitelist[] = ...

> +    switch (type) {
> +    case MODULE_LOAD_BLOCK:
> +        path = CONFIG_MODDIR "/block/";
> +        break;
> +    case MODULE_LOAD_UI:
> +        path = CONFIG_MODDIR "/ui/";
> +        break;
> +    case MODULE_LOAD_NET:
> +        path = CONFIG_MODDIR "/net/";
> +        break;

Also, separate the whitelists by type.  I.e.

  static const char * const modules_block[] = ...
  static const char * const modules_ui[] = ...
  static const char * const modules_net[] = ...

  switch (type) {
  case MODULE_LOAD_BLOCK:
    list = modules_block;
    n = ARRAY_SIZE(modules_block);
    break;
  ...
  }

No need for null termination of the array, as you're currently using.

> +    for (mp = &module_whitelist[0]; *mp; mp++) {
> +        fname = g_strdup_printf("%s%s" HOST_DSOSUF, path, *mp);
> +        module_load_file(fname);
> +        g_free(fname);
> +    }

Why this bizzare mix of g_strdup_printf and compile-time string concatenation?
 Certainly you could have arranged for HOST_DSOSUF to be built into the module
name as seen in the arrays.

Then we're back to the subdirectory vs filename prefix and CONFIG_MODDIR vs any
of several module search path options, the debate of which I don't believe has
concluded.


r~