On Fri, 09/13 07:52, Richard Henderson wrote: > On 09/13/2013 02:59 AM, Fam Zheng wrote: > > + const char *module_whitelist[] = { > > + CONFIG_MODULE_WHITELIST > > + }; > > static const char * const module_whitelist[] = ... >
OK, thanks. > > + 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. > It's that I'd like to be consistent with block whitelist code style as in bdrv_is_whitelisted(). > > + 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. > Perhaps using module type as prefix and building into whitelist can save us creating subdirs for each type and also the separation of the list as you suggested above: switch (type) { case MODULE_LOAD_BLOCK: prefix = "block-"; break; ... } for (mp = &module_whitelist[0]; *mp; mp++) { if (!strncmp(prefix, *mp, sizeof(prefix))) { /* load */ } }