[PATCH] kernel/module.c: for looping, need use 'goto' instead of 'break' to jump out in time

[PATCH] kernel/module.c: for looping, need use 'goto' instead of 'break' to jump out in time

[Chen Gang]

In the 'for' looping, when error occurs, the 'break' only jump out of
'switch', and still in 'for' looping.  If error occurs multiple times,
the original error value will be overwrite.

Currently, that will not cause issue, but still better to improve it,
so that let it return the first real error code in time.

The related commit: "1da177e Linux-2.6.12-rc"


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/module.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index b049939..7e012ff 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1977,7 +1977,7 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
 			printk("%s: please compile with -fno-common\n",
 			       mod->name);
 			ret = -ENOEXEC;
-			break;
+			goto tail;
 
 		case SHN_ABS:
 			/* Don't need to do anything */
@@ -2000,7 +2000,7 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
 			printk(KERN_WARNING "%s: Unknown symbol %s (err %li)\n",
 			       mod->name, name, PTR_ERR(ksym));
 			ret = PTR_ERR(ksym) ?: -ENOENT;
-			break;
+			goto tail;
 
 		default:
 			/* Divert to percpu allocation if a percpu var. */
@@ -2013,6 +2013,7 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
 		}
 	}
 
+tail:
 	return ret;
 }
 
-- 
1.7.7.6

Re: [PATCH] kernel/module.c: for looping, need use 'goto' instead of 'break' to jump out in time

[Rusty Russell]

Chen Gang <gang.chen@asianux.com> writes:
> In the 'for' looping, when error occurs, the 'break' only jump out of
> 'switch', and still in 'for' looping.  If error occurs multiple times,
> the original error value will be overwrite.
>
> Currently, that will not cause issue, but still better to improve it,
> so that let it return the first real error code in time.

We choose to print all the problems, rather than just one.  I don't
really mind though.

It we want this patch, it would be neater to just 'return -ENOEXEC'
and 'return PTR_ERR(ksym) ?: -ENOENT'.

Cheers,
Rusty.

> The related commit: "1da177e Linux-2.6.12-rc"
>
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/module.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index b049939..7e012ff 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1977,7 +1977,7 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>  			printk("%s: please compile with -fno-common\n",
>  			       mod->name);
>  			ret = -ENOEXEC;
> -			break;
> +			goto tail;
>  
>  		case SHN_ABS:
>  			/* Don't need to do anything */
> @@ -2000,7 +2000,7 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>  			printk(KERN_WARNING "%s: Unknown symbol %s (err %li)\n",
>  			       mod->name, name, PTR_ERR(ksym));
>  			ret = PTR_ERR(ksym) ?: -ENOENT;
> -			break;
> +			goto tail;
>  
>  		default:
>  			/* Divert to percpu allocation if a percpu var. */
> @@ -2013,6 +2013,7 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>  		}
>  	}
>  
> +tail:
>  	return ret;
>  }
>  
> -- 
> 1.7.7.6
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] kernel/module.c: for looping, need use 'goto' instead of 'break' to jump out in time

[Chen Gang]

On 2013年05月08日 08:29, Rusty Russell wrote:
> Chen Gang <gang.chen@asianux.com> writes:
>> > In the 'for' looping, when error occurs, the 'break' only jump out of
>> > 'switch', and still in 'for' looping.  If error occurs multiple times,
>> > the original error value will be overwrite.
>> >
>> > Currently, that will not cause issue, but still better to improve it,
>> > so that let it return the first real error code in time.
> We choose to print all the problems, rather than just one.  I don't
> really mind though.
> 

It sounds good: "choose to print all the problems, rather than just one"

If so, it seems enough to only return a bool value to known whether
success or fail, do not need the error details any more (since they are
already been printed)

> It we want this patch, it would be neater to just 'return -ENOEXEC'
> and 'return PTR_ERR(ksym) ?: -ENOENT'.

If we really want this patch (still only print the first error, and
return the real error value), I should send patch v2 (also 'ret' is
obsoleted)

Thanks.

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel/module.c: for looping, need use 'goto' instead of 'break' to jump out in time

[Rusty Russell]

Chen Gang <gang.chen@asianux.com> writes:
> On 2013年05月08日 08:29, Rusty Russell wrote:
>> Chen Gang <gang.chen@asianux.com> writes:
>>> > In the 'for' looping, when error occurs, the 'break' only jump out of
>>> > 'switch', and still in 'for' looping.  If error occurs multiple times,
>>> > the original error value will be overwrite.
>>> >
>>> > Currently, that will not cause issue, but still better to improve it,
>>> > so that let it return the first real error code in time.
>> We choose to print all the problems, rather than just one.  I don't
>> really mind though.
>> 
>
> It sounds good: "choose to print all the problems, rather than just one"
>
> If so, it seems enough to only return a bool value to known whether
> success or fail, do not need the error details any more (since they are
> already been printed)
>
>> It we want this patch, it would be neater to just 'return -ENOEXEC'
>> and 'return PTR_ERR(ksym) ?: -ENOENT'.
>
> If we really want this patch (still only print the first error, and
> return the real error value), I should send patch v2 (also 'ret' is
> obsoleted)

I would take such a patch, since it makes things a little neater.  But
if you don't write it, I wouldn't write it myself, since it's
borderline.

Thanks,
Rusty.

Re: [PATCH] kernel/module.c: for looping, need use 'goto' instead of 'break' to jump out in time

[Chen Gang]

On 05/13/2013 09:17 AM, Rusty Russell wrote:
> Chen Gang <gang.chen@asianux.com> writes:
>> > On 2013年05月08日 08:29, Rusty Russell wrote:
>>> >> Chen Gang <gang.chen@asianux.com> writes:
>>>>> >>> > In the 'for' looping, when error occurs, the 'break' only jump out of
>>>>> >>> > 'switch', and still in 'for' looping.  If error occurs multiple times,
>>>>> >>> > the original error value will be overwrite.
>>>>> >>> >
>>>>> >>> > Currently, that will not cause issue, but still better to improve it,
>>>>> >>> > so that let it return the first real error code in time.
>>> >> We choose to print all the problems, rather than just one.  I don't
>>> >> really mind though.
>>> >> 
>> >
>> > It sounds good: "choose to print all the problems, rather than just one"
>> >
>> > If so, it seems enough to only return a bool value to known whether
>> > success or fail, do not need the error details any more (since they are
>> > already been printed)
>> >
>>> >> It we want this patch, it would be neater to just 'return -ENOEXEC'
>>> >> and 'return PTR_ERR(ksym) ?: -ENOENT'.
>> >
>> > If we really want this patch (still only print the first error, and
>> > return the real error value), I should send patch v2 (also 'ret' is
>> > obsoleted)
> I would take such a patch, since it makes things a little neater.  But
> if you don't write it, I wouldn't write it myself, since it's
> borderline.

OK, thank, I should write it, and now I will "choose to print all the
problems, and return a bool value", and it will be as a cleanup patch.

I will finish it within 2 days (2013-05-15).

If have additional suggestions or completions, please reply.


Thanks.
-- 
Chen Gang

Asianux Corporation

[PATCH v2] kernel/module.c: cleanup patch for looping, let return 'bool' value instead of real error number.

[Chen Gang]

For looping in simplify_symbols(), when multiple errors occur, the
original error number will be override (the original related commit
"1da177e Linux-2.6.12-rc").

We focus on printing all errors as much as possible, and not focus on
the return error number (only know whether succeed or not is enough),

So when error occurs during looping, it is correct to only mark "need
return failure" and continue looping.

Since simplify_symbols() is a static function (not an API to outside),
it is better to change the function return type to 'bool', so that can
skip processing error number details incorrectly.


Signed-off-by: root <root@dhcp122.asianux.net>
---
 kernel/module.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index b049939..445a960 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1956,14 +1956,18 @@ static int verify_export_symbols(struct module *mod)
 	return 0;
 }
 
-/* Change all symbols so that st_value encodes the pointer directly. */
-static int simplify_symbols(struct module *mod, const struct load_info *info)
+/*
+ * Change all symbols so that st_value encodes the pointer directly.
+ *
+ * Return true if succeed, or false if at least 1 failure occurs.
+ */
+static bool simplify_symbols(struct module *mod, const struct load_info *info)
 {
 	Elf_Shdr *symsec = &info->sechdrs[info->index.sym];
 	Elf_Sym *sym = (void *)symsec->sh_addr;
 	unsigned long secbase;
 	unsigned int i;
-	int ret = 0;
+	bool ret = true;
 	const struct kernel_symbol *ksym;
 
 	for (i = 1; i < symsec->sh_size / sizeof(Elf_Sym); i++) {
@@ -1976,7 +1980,8 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
 			pr_debug("Common symbol: %s\n", name);
 			printk("%s: please compile with -fno-common\n",
 			       mod->name);
-			ret = -ENOEXEC;
+			/* Mark return result and continue looping */
+			ret = false;
 			break;
 
 		case SHN_ABS:
@@ -1999,7 +2004,8 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
 
 			printk(KERN_WARNING "%s: Unknown symbol %s (err %li)\n",
 			       mod->name, name, PTR_ERR(ksym));
-			ret = PTR_ERR(ksym) ?: -ENOENT;
+			/* Mark return result and continue looping */
+			ret = false;
 			break;
 
 		default:
@@ -3267,8 +3273,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	setup_modinfo(mod, info);
 
 	/* Fix up syms, so that st_value is a pointer to location. */
-	err = simplify_symbols(mod, info);
-	if (err < 0)
+	if (!simplify_symbols(mod, info))
 		goto free_modinfo;
 
 	err = apply_relocations(mod, info);
-- 
1.7.11.7

Re: [PATCH v2] kernel/module.c: cleanup patch for looping, let return 'bool' value instead of real error number.

[Chen Gang]

Oh, sorry, my fix is incorrect !!

The return value is used by caller of caller ... it seems the user mode
can sense it !! So we still should return the correct error code to
outside when error occurs.

I think we can reference 'using compiler': it prints all errors and
warnings as far as it can, but the user usually mainly focus on the
first error or warning.

So I think the suitable fix is to still print all problems, but should
return the first error code (not return the last error code).

Please help check, thanks.


On 05/13/2013 08:24 PM, Chen Gang wrote:
> 
> For looping in simplify_symbols(), when multiple errors occur, the
> original error number will be override (the original related commit
> "1da177e Linux-2.6.12-rc").
> 
> We focus on printing all errors as much as possible, and not focus on
> the return error number (only know whether succeed or not is enough),
> 
> So when error occurs during looping, it is correct to only mark "need
> return failure" and continue looping.
> 
> Since simplify_symbols() is a static function (not an API to outside),
> it is better to change the function return type to 'bool', so that can
> skip processing error number details incorrectly.
> 
> 
> Signed-off-by: root <root@dhcp122.asianux.net>
> ---
>  kernel/module.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index b049939..445a960 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1956,14 +1956,18 @@ static int verify_export_symbols(struct module *mod)
>  	return 0;
>  }
>  
> -/* Change all symbols so that st_value encodes the pointer directly. */
> -static int simplify_symbols(struct module *mod, const struct load_info *info)
> +/*
> + * Change all symbols so that st_value encodes the pointer directly.
> + *
> + * Return true if succeed, or false if at least 1 failure occurs.
> + */
> +static bool simplify_symbols(struct module *mod, const struct load_info *info)
>  {
>  	Elf_Shdr *symsec = &info->sechdrs[info->index.sym];
>  	Elf_Sym *sym = (void *)symsec->sh_addr;
>  	unsigned long secbase;
>  	unsigned int i;
> -	int ret = 0;
> +	bool ret = true;
>  	const struct kernel_symbol *ksym;
>  
>  	for (i = 1; i < symsec->sh_size / sizeof(Elf_Sym); i++) {
> @@ -1976,7 +1980,8 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>  			pr_debug("Common symbol: %s\n", name);
>  			printk("%s: please compile with -fno-common\n",
>  			       mod->name);
> -			ret = -ENOEXEC;
> +			/* Mark return result and continue looping */
> +			ret = false;
>  			break;
>  
>  		case SHN_ABS:
> @@ -1999,7 +2004,8 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>  
>  			printk(KERN_WARNING "%s: Unknown symbol %s (err %li)\n",
>  			       mod->name, name, PTR_ERR(ksym));
> -			ret = PTR_ERR(ksym) ?: -ENOENT;
> +			/* Mark return result and continue looping */
> +			ret = false;
>  			break;
>  
>  		default:
> @@ -3267,8 +3273,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	setup_modinfo(mod, info);
>  
>  	/* Fix up syms, so that st_value is a pointer to location. */
> -	err = simplify_symbols(mod, info);
> -	if (err < 0)
> +	if (!simplify_symbols(mod, info))
>  		goto free_modinfo;
>  
>  	err = apply_relocations(mod, info);
> 


-- 
Chen Gang

Asianux Corporation

[PATCH v3] kernel/module.c: need return the first error code to upper caller when error occurs

[Chen Gang]

When multiple errors occur, simplify_symbols() will return the last
error code to the upper caller.

In this case, better to return the first error code to the upper caller.

Just like "using compiler": it will print all errors and warnings as
much as it can, but the user usually mainly focus on the first error or
warning. Since 'user' only can get one error return code, 'he/she'
usually assume the error code will match the first error print line.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/module.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index e4ee1bf..9e6c96d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1976,7 +1976,8 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
 			pr_debug("Common symbol: %s\n", name);
 			printk("%s: please compile with -fno-common\n",
 			       mod->name);
-			ret = -ENOEXEC;
+			if (!ret)
+				ret = -ENOEXEC;
 			break;
 
 		case SHN_ABS:
@@ -1999,7 +2000,8 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
 
 			printk(KERN_WARNING "%s: Unknown symbol %s (err %li)\n",
 			       mod->name, name, PTR_ERR(ksym));
-			ret = PTR_ERR(ksym) ?: -ENOENT;
+			if (!ret)
+				ret = PTR_ERR(ksym) ?: -ENOENT;
 			break;
 
 		default:
-- 
1.7.7.6

Re: [PATCH v3] kernel/module.c: need return the first error code to upper caller when error occurs

[Chen Gang]

Hello Maintainers:

Please help check the patch whether OK or not, when you have time.


Thanks.

On 05/17/2013 12:33 PM, Chen Gang wrote:
> 
> When multiple errors occur, simplify_symbols() will return the last
> error code to the upper caller.
> 
> In this case, better to return the first error code to the upper caller.
> 
> Just like "using compiler": it will print all errors and warnings as
> much as it can, but the user usually mainly focus on the first error or
> warning. Since 'user' only can get one error return code, 'he/she'
> usually assume the error code will match the first error print line.
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/module.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index e4ee1bf..9e6c96d 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1976,7 +1976,8 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>  			pr_debug("Common symbol: %s\n", name);
>  			printk("%s: please compile with -fno-common\n",
>  			       mod->name);
> -			ret = -ENOEXEC;
> +			if (!ret)
> +				ret = -ENOEXEC;
>  			break;
>  
>  		case SHN_ABS:
> @@ -1999,7 +2000,8 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>  
>  			printk(KERN_WARNING "%s: Unknown symbol %s (err %li)\n",
>  			       mod->name, name, PTR_ERR(ksym));
> -			ret = PTR_ERR(ksym) ?: -ENOENT;
> +			if (!ret)
> +				ret = PTR_ERR(ksym) ?: -ENOENT;
>  			break;
>  
>  		default:
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH v3] kernel/module.c: need return the first error code to upper caller when error occurs

[Rusty Russell]

Chen Gang <gang.chen@asianux.com> writes:
> Hello Maintainers:
>
> Please help check the patch whether OK or not, when you have time.

Hi Chen,

        There's nothing *wrong* with the patch, but I don't think it's
worthwhile.  If this were your very first kernel patch, I'd probably
apply it just to encourage you, but you're not a newbie any more!

Cheers,
Rusty.

> On 05/17/2013 12:33 PM, Chen Gang wrote:
>> 
>> When multiple errors occur, simplify_symbols() will return the last
>> error code to the upper caller.
>> 
>> In this case, better to return the first error code to the upper caller.
>> 
>> Just like "using compiler": it will print all errors and warnings as
>> much as it can, but the user usually mainly focus on the first error or
>> warning. Since 'user' only can get one error return code, 'he/she'
>> usually assume the error code will match the first error print line.
>> 
>> 
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  kernel/module.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/kernel/module.c b/kernel/module.c
>> index e4ee1bf..9e6c96d 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1976,7 +1976,8 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>>  			pr_debug("Common symbol: %s\n", name);
>>  			printk("%s: please compile with -fno-common\n",
>>  			       mod->name);
>> -			ret = -ENOEXEC;
>> +			if (!ret)
>> +				ret = -ENOEXEC;
>>  			break;
>>  
>>  		case SHN_ABS:
>> @@ -1999,7 +2000,8 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>>  
>>  			printk(KERN_WARNING "%s: Unknown symbol %s (err %li)\n",
>>  			       mod->name, name, PTR_ERR(ksym));
>> -			ret = PTR_ERR(ksym) ?: -ENOENT;
>> +			if (!ret)
>> +				ret = PTR_ERR(ksym) ?: -ENOENT;
>>  			break;
>>  
>>  		default:
>> 
>
>
> -- 
> Chen Gang
>
> Asianux Corporation

Re: [PATCH v3] kernel/module.c: need return the first error code to upper caller when error occurs

[Chen Gang]

On 05/23/2013 11:54 AM, Rusty Russell wrote:
> Chen Gang <gang.chen@asianux.com> writes:
>> > Hello Maintainers:
>> >
>> > Please help check the patch whether OK or not, when you have time.
> Hi Chen,
> 
>         There's nothing *wrong* with the patch, but I don't think it's
> worthwhile.  If this were your very first kernel patch, I'd probably
> apply it just to encourage you, but you're not a newbie any more!

OK, I can understand.


Thanks.
-- 
Chen Gang

Asianux Corporation