2020-08-13 12:49:18

by Youling Tang

Subject: [PATCH] kexec: Delete an unnecessary comparison

Regardless of whether the ret value is zero or non-zero, the trajectory
of the program execution is the same, so there is no need to compare.

Signed-off-by: Youling Tang <[email protected]>
---
kernel/kexec_file.c | 2 --
1 file changed, 2 deletions(-)

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 78c0837..3ad0ae2 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -800,8 +800,6 @@ static int kexec_calculate_store_digests(struct kimage *image)

ret = kexec_purgatory_get_set_symbol(image, "purgatory_sha256_digest",
digest, SHA256_DIGEST_SIZE, 0);
- if (ret)
- goto out_free_digest;
}

out_free_digest:
--
2.1.0

2020-08-14 05:55:37

by Dave Young

Subject: Re: [PATCH] kexec: Delete an unnecessary comparison

On 08/13/20 at 08:45pm, Youling Tang wrote:
> Regardless of whether the ret value is zero or non-zero, the trajectory
> of the program execution is the same, so there is no need to compare.
>
> Signed-off-by: Youling Tang <[email protected]>
> ---
> kernel/kexec_file.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 78c0837..3ad0ae2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -800,8 +800,6 @@ static int kexec_calculate_store_digests(struct kimage *image)
>
> ret = kexec_purgatory_get_set_symbol(image, "purgatory_sha256_digest",
> digest, SHA256_DIGEST_SIZE, 0);
> - if (ret)
> - goto out_free_digest;
> }
>
> out_free_digest:
> --
> 2.1.0
>

Acked-by: Dave Young <[email protected]>

Thanks
Dave

2020-08-14 06:08:17

by Joe Perches

Subject: Re: [PATCH] kexec: Delete an unnecessary comparison

On Thu, 2020-08-13 at 20:45 +0800, Youling Tang wrote:
> Regardless of whether the ret value is zero or non-zero, the trajectory
> of the program execution is the same, so there is no need to compare.
>
> Signed-off-by: Youling Tang <[email protected]>
> ---
> kernel/kexec_file.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 78c0837..3ad0ae2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -800,8 +800,6 @@ static int kexec_calculate_store_digests(struct kimage *image)
>
> ret = kexec_purgatory_get_set_symbol(image, "purgatory_sha256_digest",
> digest, SHA256_DIGEST_SIZE, 0);
> - if (ret)
> - goto out_free_digest;
> }
>
> out_free_digest:

If you really want to change the function, then
you could change a couple of breaks to gotos,
remove multiple unnecessary tests, and unindent
a block of code too.

---
kernel/kexec_file.c | 30 ++++++++++++------------------
1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index ca40bef75a61..34a025e85887 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -763,7 +763,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
ret = crypto_shash_update(desc, ksegment->kbuf,
ksegment->bufsz);
if (ret)
- break;
+ goto out_free_digest;

/*
* Assume rest of the buffer is filled with zero and
@@ -777,32 +777,26 @@ static int kexec_calculate_store_digests(struct kimage *image)
bytes = zero_buf_sz;
ret = crypto_shash_update(desc, zero_buf, bytes);
if (ret)
- break;
+ goto out_free_digest;
nullsz -= bytes;
}

- if (ret)
- break;
-
sha_regions[j].start = ksegment->mem;
sha_regions[j].len = ksegment->memsz;
j++;
}

- if (!ret) {
- ret = crypto_shash_final(desc, digest);
- if (ret)
- goto out_free_digest;
- ret = kexec_purgatory_get_set_symbol(image, "purgatory_sha_regions",
- sha_regions, sha_region_sz, 0);
- if (ret)
- goto out_free_digest;
+ ret = crypto_shash_final(desc, digest);
+ if (ret)
+ goto out_free_digest;

- ret = kexec_purgatory_get_set_symbol(image, "purgatory_sha256_digest",
- digest, SHA256_DIGEST_SIZE, 0);
- if (ret)
- goto out_free_digest;
- }
+ ret = kexec_purgatory_get_set_symbol(image, "purgatory_sha_regions",
+ sha_regions, sha_region_sz, 0);
+ if (ret)
+ goto out_free_digest;
+
+ ret = kexec_purgatory_get_set_symbol(image, "purgatory_sha256_digest",
+ digest, SHA256_DIGEST_SIZE, 0);

out_free_digest:
kfree(digest);

2020-08-14 10:17:23

by Youling Tang

Subject: Re: [PATCH] kexec: Delete an unnecessary comparison

On 08/14/2020 02:07 PM, Joe Perches wrote:
> On Thu, 2020-08-13 at 20:45 +0800, Youling Tang wrote:
>> Regardless of whether the ret value is zero or non-zero, the trajectory
>> of the program execution is the same, so there is no need to compare.
>>
>> Signed-off-by: Youling Tang <[email protected]>
>> ---
>> kernel/kexec_file.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
>> index 78c0837..3ad0ae2 100644
>> --- a/kernel/kexec_file.c
>> +++ b/kernel/kexec_file.c
>> @@ -800,8 +800,6 @@ static int kexec_calculate_store_digests(struct kimage *image)
>>
>> ret = kexec_purgatory_get_set_symbol(image, "purgatory_sha256_digest",
>> digest, SHA256_DIGEST_SIZE, 0);
>> - if (ret)
>> - goto out_free_digest;
>> }
>>
>> out_free_digest:
> If you really want to change the function, then
> you could change a couple of breaks to gotos,
> remove multiple unnecessary tests, and unindent
> a block of code too.
>
> ---
> kernel/kexec_file.c | 30 ++++++++++++------------------
> 1 file changed, 12 insertions(+), 18 deletions(-)
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index ca40bef75a61..34a025e85887 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -763,7 +763,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
> ret = crypto_shash_update(desc, ksegment->kbuf,
> ksegment->bufsz);
> if (ret)
> - break;
> + goto out_free_digest;
>
> /*
> * Assume rest of the buffer is filled with zero and
> @@ -777,32 +777,26 @@ static int kexec_calculate_store_digests(struct kimage *image)
> bytes = zero_buf_sz;
> ret = crypto_shash_update(desc, zero_buf, bytes);
> if (ret)
> - break;
> + goto out_free_digest;
> nullsz -= bytes;
> }
>
> - if (ret)
> - break;
> -
> sha_regions[j].start = ksegment->mem;
> sha_regions[j].len = ksegment->memsz;
> j++;
> }
>
> - if (!ret) {
> - ret = crypto_shash_final(desc, digest);
> - if (ret)
> - goto out_free_digest;
> - ret = kexec_purgatory_get_set_symbol(image, "purgatory_sha_regions",
> - sha_regions, sha_region_sz, 0);
> - if (ret)
> - goto out_free_digest;
> + ret = crypto_shash_final(desc, digest);
> + if (ret)
> + goto out_free_digest;
>
> - ret = kexec_purgatory_get_set_symbol(image, "purgatory_sha256_digest",
> - digest, SHA256_DIGEST_SIZE, 0);
> - if (ret)
> - goto out_free_digest;
> - }
> + ret = kexec_purgatory_get_set_symbol(image, "purgatory_sha_regions",
> + sha_regions, sha_region_sz, 0);
> + if (ret)
> + goto out_free_digest;
> +
> + ret = kexec_purgatory_get_set_symbol(image, "purgatory_sha256_digest",
> + digest, SHA256_DIGEST_SIZE, 0);
>
> out_free_digest:
> kfree(digest);
>
OK, looks good to me, I will send v2.

Thanks,
Youling