Re: Warn on macros with flow control statements

[Julia Lawall <julia.lawall@inria.fr>]

On Thu, 16 Mar 2023, Sumitra Sharma wrote:

>
> Hi
>
> I am working on the below chechpatch.pl check
>
> WARNING: Macros with flow control statements should be avoided
> #42: FILE: ./drivers/staging/qlge/qlge_devlink.c:42:
> +#define FILL_SEG(seg_hdr, seg_regs)                                        \
> +       do {                                                                \
> +               err = qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
> +               if (err) {                                                  \
> +                       kvfree(dump);                                       \
> +                       return err;                                         \
> +               }                                                           \
> +       } while (0)
>
>
> I was trying to change this macro into an inline function like this
>
> static inline int FILL_SEG(struct devlink_fmsg *fmsg, struct mpi_coredump_segment_header *seg_hdr, u32 *seg_regs)
> {
>         int err =0;
>
>         err = qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs);
>         if (err) {
>                 kvfree(dump);
>                 return err;
>         }
> }

This is not a promising approach.  The return that you have here will just
return from the function that you have defined, while the return in the
macro was intended to return from the function that calls the macro.

The problem with the macro is that when you look at the uses, you don't
know that a return is possible.  But given that the macro is defined right
next to the function that uses it, and that somehow extracting the return
from the macro would be hackish and complex, the code is probably ok as it
is.

The macro is pretty unpleasant, also because it captures several variables
(fmsg and dump).

Maybe a solution would be to replace all the macro calls with:

err = err || qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs);

and then test for a non zero err value at the end.  But maybe wait for
someone else to weigh in before taking that approach..

julia

>
> But I am not sure whether I am going in the right direction..or how to
> move further in it. I am looking for some guidance.
>
>
>
> Regards,
>
> Sumitra
>
>