On Thu, 14 Dec 2023 at 23:32, Samuel Tardieu <s...@rfc1149.net> wrote:
>
> `buf_rw` is always `NULL` when jumping to the `fail` label. Move the
> label `down` after the `if (buf_rw) { ... }` statement.
>
> Signed-off-by: Samuel Tardieu <s...@rfc1149.net>
> ---
> tcg/region.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tcg/region.c b/tcg/region.c
> index 6d657e8c33..691a726eae 100644
> --- a/tcg/region.c
> +++ b/tcg/region.c
> @@ -596,10 +596,10 @@ static int alloc_code_gen_buffer_splitwx_memfd(size_t
> size, Error **errp)
>
> fail_rx:
> error_setg_errno(errp, errno, "failed to map shared memory for execute");
> - fail:
> if (buf_rw) {
> munmap(buf_rw, size);
> }
> + fail:
> if (fd >= 0) {
> close(fd);
> }
It's also the case that fd is always -1 when we jump
to the 'fail' label, so if we're moving it down then
we should move it past that as well.
At this point you might as well make the check after
qemu_memfd_alloc() just be
if (buf_rw == NULL) {
return -1;
}
and drop the 'fail:' label entirely. And then we
know that in this code path buf_rw must be non-NULL
and fd must be >= 0, so the fail_rx: codepath doesn't
need to explicitly test those.
So, well, all of this is definitely removing dead
code, but on the other hand it's also moving away
from the coding-style pattern the function has at
the moment, which is "there is a fail-and-exit
codepath which is robust against wherever you might
choose to jump to it, and so if we need to add new
code to this function then it also can jump to 'fail'
without any further updates to that error-exit path".
Instead we end up with an "every error-exit check
does its own tidyup" idiom. For the sake of not having
a static checker say "this is technically dead code",
is that worth doing, or does it make the code a little
less readable and less amenable to future modification?
I'm not sure...
-- PMM
