Re: [PATCH] util: Remove redundant checks in the openpty()

[Peter Maydell]

On Sat, 31 Oct 2020 at 11:04, AlexChen <alex.chen@huawei.com> wrote:
>
> As we can see from the following function call stack, the amaster and the 
> aslave
> cannot be NULL: char_pty_open() -> qemu_openpty_raw() -> openpty().
> In addition, the amaster and the aslave has been dereferenced at the beginning
> of the openpty(). So the checks on amaster and aslave in the openpty() are 
> redundant.
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Alex Chen <alex.chen@huawei.com>

This function is trying to match the BSD/glibc openpty()
function, so the thing to check here is not QEMU's specific
current usage but the API specification for openpty():
https://www.gnu.org/software/libc/manual/html_node/Pseudo_002dTerminal-Pairs.html
https://www.freebsd.org/cgi/man.cgi?query=openpty

The spec says that name, termp and winp can all be
NULL, but it doesn't say this for amaster and aslave,
so indeed the change in this patch is the correct one.

> ---
>  util/qemu-openpty.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/util/qemu-openpty.c b/util/qemu-openpty.c
> index eb17f5b0bc..427f43a769 100644
> --- a/util/qemu-openpty.c
> +++ b/util/qemu-openpty.c
> @@ -80,10 +80,9 @@ static int openpty(int *amaster, int *aslave, char *name,
>              (termp != NULL && tcgetattr(sfd, termp) < 0))
>                  goto err;
>
> -        if (amaster)
> -                *amaster = mfd;
> -        if (aslave)
> -                *aslave = sfd;
> +        *amaster = mfd;
> +        *aslave = sfd;
> +
>          if (winp)
>                  ioctl(sfd, TIOCSWINSZ, winp);

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

though you might like to mention in the commit message that
the openpty() API doesn't allow NULL amaster or aslave
arguments.

thanks
-- PMM