Re: [PATCH] common/efi: deviate Rule 2.1 violation in read_file()

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Aug 21, 2025 at 11:28:01AM +0000, Dmytro Prokopchuk1 wrote:
> 
> 
> On 8/21/25 13:33, Jan Beulich wrote:
> > On 20.08.2025 20:05, Dmytro Prokopchuk1 wrote:
> >> --- a/xen/common/efi/boot.c
> >> +++ b/xen/common/efi/boot.c
> >> @@ -852,7 +852,7 @@ static bool __init read_file(EFI_FILE_HANDLE 
> >> dir_handle, CHAR16 *name,
> >>       PrintErr(L" failed for ");
> >>       PrintErrMesg(name, ret);
> >>   
> >> -    /* not reached */
> >> +    /* SAF-15-safe deliberately unreachable code */
> >>       return false;
> >>   }
> > 
> > Much better (even if not tagged as v2). Yet then, did you consider
> > alternatives? For example, with PrintErrMesg() properly annotated 
> > "noreturn",
> > I'd kind of expect compilers to not object to the omission of the "return"
> > statement here. This would then let us get away without a new SAF comment.
> > While you explain in the SAF text why you retain the statement, I'm not
> > convinced of code clarity suffering if it was deleted, as long as a suitable
> > comment is still there. If PrintErrMesg() lost its "noreturn", surely
> > compilers would then diagnose the lack of "return".
> > 
> > Jan
> 
> Sure, the next version will be v3.
> Actually, the PrintErrMesg() already has property 'noreturn'.
> And it really gives an alternative way: remove 'return false;' from the 
> function read_file() (leaving comment there).
> 
> With that change Misra is "happy".
> 
> In case of removing 'noreturn' attribute from PrintErrMesg() function 
> compiler will detect that:
> arch/arm/efi/boot.c: In function ‘read_file’:
> arch/arm/efi/boot.c:854:1: error: control reaches end of non-void 
> function [-Werror=return-type]
>   }
>   ^
> 
> Is it OK to prepare such ^ patch?

IMO sounds like the best solution for this issue.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature