linux-kernel - Re: [PATCH] kernel:module Fix coding style errors and warnings.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87tx33aa37.fsf@rustcorp.com.au>
Date:	Fri, 17 Oct 2014 09:36:20 +1030
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Ionut Alexa <ionut.m.alexa@...il.com>
Cc:	linux-kernel@...r.kernel.org, Ionut Alexa <ionut.m.alexa@...il.com>
Subject: Re: [PATCH] kernel:module Fix coding style errors and warnings.

Ionut Alexa <ionut.m.alexa@...il.com> writes:
> Fixed codin style errors and warnings. Changes printk with
> print_debug/warn. Changed seq_printf to seq_puts.
>
> Signed-off-by: Ionut Alexa <ionut.m.alexa@...il.com>

Hi Ionut,

Please drop the following changes:
> @@ -110,7 +110,7 @@ struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */
>  #ifdef CONFIG_MODULE_SIG_FORCE
>  static bool sig_enforce = true;
>  #else
> -static bool sig_enforce = false;
> +static bool sig_enforce; /* by default set to false */
>  
>  static int param_set_bool_enable_only(const char *val,
>  				      const struct kernel_param *kp)
> @@ -156,15 +156,15 @@ static BLOCKING_NOTIFIER_HEAD(module_notify_list);
>  
>  /* Bounds of module allocation, for speeding __module_address.
>   * Protected by module_mutex. */
> -static unsigned long module_addr_min = -1UL, module_addr_max = 0;
> +static unsigned long module_addr_min = -1UL, module_addr_max; /* addr_max=0 */

I think the explicit initializers are clearer.  Gcc realizes they're
zero and puts them in bss anyway, so there's no size cost.

> -int register_module_notifier(struct notifier_block * nb)
> +int register_module_notifier(struct notifier_block *nb)
>  {
>  	return blocking_notifier_chain_register(&module_notify_list, nb);
>  }
>  EXPORT_SYMBOL(register_module_notifier);
>  
> -int unregister_module_notifier(struct notifier_block * nb)
> +int unregister_module_notifier(struct notifier_block *nb)
>  {
>  	return blocking_notifier_chain_unregister(&module_notify_list, nb);
>  }
> @@ -740,8 +740,7 @@ static inline int try_force_unload(unsigned int flags)
>  }
>  #endif /* CONFIG_MODULE_FORCE_UNLOAD */
>  
> -struct stopref
> -{
> +struct stopref {
>  	struct module *mod;
>  	int flags;
>  	int *forced;

These are fine.

> @@ -878,7 +877,7 @@ static inline void print_unload_info(struct seq_file *m, struct module *mod)
>  	seq_printf(m, " %lu ", module_refcount(mod));
>  
>  	/* Always include a trailing , so userspace can differentiate
> -           between this and the old multi-field proc format. */
> +	 * between this and the old multi-field proc format. */
>  	list_for_each_entry(use, &mod->source_list, source_list) {
>  		printed_something = 1;
>  		seq_printf(m, "%s,", use->source->name);

Actually, kernel style for multi-line comments, like it or not, is:

  	/*
         * Always include a trailing , so userspace can differentiate
	 * between this and the old multi-field proc format.
         */
        
> @@ -1953,7 +1951,7 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>  			/* We compiled with -fno-common.  These are not
>  			   supposed to happen.  */
>  			pr_debug("Common symbol: %s\n", name);
> -			printk("%s: please compile with -fno-common\n",
> +			pr_debug("%s: please compile with -fno-common\n",
>  			       mod->name);
>  			ret = -ENOEXEC;
>  			break;

Please change it to pr_warn rather than pr_debug!

> @@ -3022,7 +3020,7 @@ static int do_init_module(struct module *mod)
>  		ret = do_one_initcall(mod->init);
>  	if (ret < 0) {
>  		/* Init routine failed: abort.  Try to protect us from
> -                   buggy refcounters. */
> +		 * buggy refcounters. */
>  		mod->state = MODULE_STATE_GOING;
>  		synchronize_sched();
>  		module_put(mod);
> @@ -3174,7 +3172,7 @@ out:

Comment style here, too.

> @@ -3816,7 +3814,7 @@ void print_modules(void)
>  	struct module *mod;
>  	char buf[8];
>  
> -	printk(KERN_DEFAULT "Modules linked in:");
> +	pr_warn("Modules linked in:");

This is not the same as KERN_DEFAULT; is it correct?

Thanks,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/