1. Patchew
  2. Xen
  3. View series

[PATCH v3] common/efi: fix Rule 2.1 violation in read_file()

Dmytro Prokopchuk1 posted 1 patch 2 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/4a1a4a3406d227348afa1ad2ce90dc5264fdb44a.1755783750.git.dmytro._5Fprokopchuk1@epam.com
There is a newer version of this series
xen/common/efi/boot.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
[PATCH v3] common/efi: fix Rule 2.1 violation in read_file()
Posted by Dmytro Prokopchuk1 2 months, 1 week ago 
MISRA C Rule 2.1 states: "A project shall not contain unreachable code."

The return statements in the 'read_file()' function is unreachable due
to function 'PrintErrMesg()' which has 'noreturn' attribute:
        PrintErrMesg(name, ret);
        /* not reached */
        return false;
    }

No explicit return statement is needed here. Remove the statement and
write a justification comment instead. No functional changes.

Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
---
Link to v2:
https://patchew.org/Xen/c20a58f24875806adfaf491f9c6eef2ca8682d18.1755711594.git.dmytro._5Fprokopchuk1@epam.com/

Changes in v3:
- removed unreachable code instead of deviation
- updated commit subject and message

Test CI pipeline:
https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/1996439444
---
 xen/common/efi/boot.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 50ff1d1bd2..325de05b18 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -851,9 +851,13 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     PrintErr(what);
     PrintErr(L" failed for ");
     PrintErrMesg(name, ret);
-
-    /* not reached */
-    return false;
+    /*
+     * No explicit return statement is needed here because 'PrintErrMesg()' is
+     * marked as 'noreturn', which guarantees that it never returns control to
+     * the caller. If the 'noreturn' attribute of 'PrintErrMesg()' is removed
+     * in the future, compiler will emit an error about the missing return
+     * statement (build-time safeguard).
+     */
 }
 
 static bool __init read_section(const EFI_LOADED_IMAGE *image,
-- 
2.43.0
Re: [PATCH v3] common/efi: fix Rule 2.1 violation in read_file()
Posted by Marek Marczykowski-Górecki 2 months, 1 week ago 
On Thu, Aug 21, 2025 at 01:56:28PM +0000, Dmytro Prokopchuk1 wrote:
> MISRA C Rule 2.1 states: "A project shall not contain unreachable code."
> 
> The return statements in the 'read_file()' function is unreachable due
> to function 'PrintErrMesg()' which has 'noreturn' attribute:
>         PrintErrMesg(name, ret);
>         /* not reached */
>         return false;
>     }
> 
> No explicit return statement is needed here. Remove the statement and
> write a justification comment instead. No functional changes.
> 
> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
> ---
> Link to v2:
> https://patchew.org/Xen/c20a58f24875806adfaf491f9c6eef2ca8682d18.1755711594.git.dmytro._5Fprokopchuk1@epam.com/
> 
> Changes in v3:
> - removed unreachable code instead of deviation
> - updated commit subject and message
> 
> Test CI pipeline:
> https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/1996439444
> ---
>  xen/common/efi/boot.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 50ff1d1bd2..325de05b18 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -851,9 +851,13 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>      PrintErr(what);
>      PrintErr(L" failed for ");
>      PrintErrMesg(name, ret);
> -
> -    /* not reached */
> -    return false;
> +    /*
> +     * No explicit return statement is needed here because 'PrintErrMesg()' is
> +     * marked as 'noreturn', which guarantees that it never returns control to
> +     * the caller. If the 'noreturn' attribute of 'PrintErrMesg()' is removed
> +     * in the future, compiler will emit an error about the missing return
> +     * statement (build-time safeguard).
> +     */

I don't think this verbose code comment is needed here. Other similar places
use simply "Doesn't return." next to the function call, or nothing at
all if the function name already suggests it (which IMO is not the case
here). The longer explanation may be put in the commit message.

With that addressed:

Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Re: [PATCH v3] common/efi: fix Rule 2.1 violation in read_file()
Posted by Jan Beulich 2 months, 1 week ago 
On 21.08.2025 16:24, Marek Marczykowski-Górecki wrote:
> On Thu, Aug 21, 2025 at 01:56:28PM +0000, Dmytro Prokopchuk1 wrote:
>> MISRA C Rule 2.1 states: "A project shall not contain unreachable code."
>>
>> The return statements in the 'read_file()' function is unreachable due
>> to function 'PrintErrMesg()' which has 'noreturn' attribute:
>>         PrintErrMesg(name, ret);
>>         /* not reached */
>>         return false;
>>     }
>>
>> No explicit return statement is needed here. Remove the statement and
>> write a justification comment instead. No functional changes.
>>
>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
>> ---
>> Link to v2:
>> https://patchew.org/Xen/c20a58f24875806adfaf491f9c6eef2ca8682d18.1755711594.git.dmytro._5Fprokopchuk1@epam.com/
>>
>> Changes in v3:
>> - removed unreachable code instead of deviation
>> - updated commit subject and message
>>
>> Test CI pipeline:
>> https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/1996439444
>> ---
>>  xen/common/efi/boot.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>> index 50ff1d1bd2..325de05b18 100644
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -851,9 +851,13 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>>      PrintErr(what);
>>      PrintErr(L" failed for ");
>>      PrintErrMesg(name, ret);
>> -
>> -    /* not reached */
>> -    return false;
>> +    /*
>> +     * No explicit return statement is needed here because 'PrintErrMesg()' is
>> +     * marked as 'noreturn', which guarantees that it never returns control to
>> +     * the caller. If the 'noreturn' attribute of 'PrintErrMesg()' is removed
>> +     * in the future, compiler will emit an error about the missing return
>> +     * statement (build-time safeguard).
>> +     */
> 
> I don't think this verbose code comment is needed here. Other similar places
> use simply "Doesn't return." next to the function call, or nothing at
> all if the function name already suggests it (which IMO is not the case
> here).

Or simply keep the comment that was already there?

Jan

> The longer explanation may be put in the commit message.
> 
> With that addressed:
> 
> Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>
Re: [PATCH v3] common/efi: fix Rule 2.1 violation in read_file()
Posted by Dmytro Prokopchuk1 2 months, 1 week ago 

On 8/21/25 17:26, Jan Beulich wrote:
> On 21.08.2025 16:24, Marek Marczykowski-Górecki wrote:
>> On Thu, Aug 21, 2025 at 01:56:28PM +0000, Dmytro Prokopchuk1 wrote:
>>> MISRA C Rule 2.1 states: "A project shall not contain unreachable code."
>>>
>>> The return statements in the 'read_file()' function is unreachable due
>>> to function 'PrintErrMesg()' which has 'noreturn' attribute:
>>>          PrintErrMesg(name, ret);
>>>          /* not reached */
>>>          return false;
>>>      }
>>>
>>> No explicit return statement is needed here. Remove the statement and
>>> write a justification comment instead. No functional changes.
>>>
>>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
>>> ---
>>> Link to v2:
>>> https://patchew.org/Xen/c20a58f24875806adfaf491f9c6eef2ca8682d18.1755711594.git.dmytro._5Fprokopchuk1@epam.com/
>>>
>>> Changes in v3:
>>> - removed unreachable code instead of deviation
>>> - updated commit subject and message
>>>
>>> Test CI pipeline:
>>> https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/1996439444
>>> ---
>>>   xen/common/efi/boot.c | 10 +++++++---
>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>>> index 50ff1d1bd2..325de05b18 100644
>>> --- a/xen/common/efi/boot.c
>>> +++ b/xen/common/efi/boot.c
>>> @@ -851,9 +851,13 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>>>       PrintErr(what);
>>>       PrintErr(L" failed for ");
>>>       PrintErrMesg(name, ret);
>>> -
>>> -    /* not reached */
>>> -    return false;
>>> +    /*
>>> +     * No explicit return statement is needed here because 'PrintErrMesg()' is
>>> +     * marked as 'noreturn', which guarantees that it never returns control to
>>> +     * the caller. If the 'noreturn' attribute of 'PrintErrMesg()' is removed
>>> +     * in the future, compiler will emit an error about the missing return
>>> +     * statement (build-time safeguard).
>>> +     */
>>
>> I don't think this verbose code comment is needed here. Other similar places
>> use simply "Doesn't return." next to the function call, or nothing at
>> all if the function name already suggests it (which IMO is not the case
>> here).
>
> Or simply keep the comment that was already there?
>
> Jan

Anyway, comments "Doesn't return." and "not reached" are almost the same.
To simplify patch, I'm going to leave old comment "not reached" and move
description into commit message.

Dmytro.

>
>> The longer explanation may be put in the commit message.
>>
>> With that addressed:
>>
>> Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.