Received-SPF: pass (google.com: domain of linux-kernel+bounces-180265-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240515010805.605511-1-xandfury@gmail.com> <20240515010805.605511-4-xandfury@gmail.com>
In-Reply-To: <20240515010805.605511-4-xandfury@gmail.com>
From: Bill Wendling <morbo@google.com>
Date: Wed, 15 May 2024 11:32:48 -0700
Message-ID: <CAGG=3QWyeWuLmJg1R7X8T4CsSgM+EiDToxjd_-8MD94D=DCLtw@mail.gmail.com>
Subject: Re: [RFC PATCH 3/4] selftests/binderfs: Macro argument 'fd' may be
 better as '(fd)' to avoid precedence issues
To: Abhinav Saxena <xandfury@gmail.com>
Cc: linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org, 
	llvm@lists.linux.dev, Shuah Khan <shuah@kernel.org>, 
	Nathan Chancellor <nathan@kernel.org>, Nick Desaulniers <ndesaulniers@google.com>, 
	Justin Stitt <justinstitt@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, May 14, 2024 at 6:08=E2=80=AFPM Abhinav Saxena <xandfury@gmail.com>=
 wrote:
>
> Change the macro argument 'fd' to '(fd)' to avoid potential precedence
> issues. Without parentheses, the macro expansion could lead to
> unexpected behavior when used with an expression having different
> precedence levels.
>
> Example Code:
>
>     #define CALC_SQR_BAD(x) (x*x)
>     #define CALC_SQR_GOOD(x) ((x)*(x))
>
> CALC_SQR_BAD(2+3) expands to 2+(3*3)+2, giving 11 as the incorrect
> answer. Enclosing with parathesis CALC_SQR_GOOD(2+3) sets the correct

s/parathesis/parentheses/

> order of precedence and expands to (2+3)*(2+3), giving 25 as the
> correct answer.
>
> Signed-off-by: Abhinav Saxena <xandfury@gmail.com>
> ---
>  .../testing/selftests/filesystems/binderfs/binderfs_test.c  | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c=
 b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> index 4a149e3d4ba4..d5dba6c1e07f 100644
> --- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> @@ -30,11 +30,11 @@
>
>  #define close_prot_errno_disarm(fd)      \
>         do {                             \
> -               if (fd >=3D 0) {           \
> +               if ((fd) >=3D 0) {         \
>                         int _e_ =3D errno; \
> -                       close(fd);       \
> +                       close((fd));     \
>                         errno =3D _e_;     \
> -                       fd =3D -EBADF;     \
> +                       (fd) =3D -EBADF;   \

While I agree that it's generally good to add parentheses in macros,
this line ensures that 'fd' must be an lvalue and not an arithmetic
expression.

-bw

>                 }                        \
>         } while (false)
>
> --
> 2.34.1
>