Re: [PATCH v1 5/9] gdbstub: eliminate gdbserver_fd global

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 4/30/20 9:01 PM, Alex Bennée wrote:
> We don't really need to track this fd beyond the initial creation of
> the socket. We already know if the system has been initialised by
> virtue of the gdbserver_state so lets remove it. This makes the later
> re-factoring easier.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v1
>    - fix coding style issue
> ---
>   gdbstub.c | 24 +++++++++++-------------
>   1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 171e150950..b5381aa520 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -398,8 +398,6 @@ static void reset_gdbserver_state(void)
>   bool gdb_has_xml;
>   
>   #ifdef CONFIG_USER_ONLY
> -/* XXX: This is not thread safe.  Do we care?  */
> -static int gdbserver_fd = -1;
>   
>   static int get_char(void)
>   {
> @@ -2964,7 +2962,7 @@ void gdb_exit(CPUArchState *env, int code)
>         return;
>     }
>   #ifdef CONFIG_USER_ONLY
> -  if (gdbserver_fd < 0 || gdbserver_state.fd < 0) {
> +  if (gdbserver_state.fd < 0) {
>         return;
>     }
>   #endif
> @@ -3011,7 +3009,7 @@ gdb_handlesig(CPUState *cpu, int sig)
>       char buf[256];
>       int n;
>   
> -    if (gdbserver_fd < 0 || gdbserver_state.fd < 0) {
> +    if (!gdbserver_state.init || gdbserver_state.fd < 0) {
>           return sig;
>       }
>   
> @@ -3060,7 +3058,7 @@ void gdb_signalled(CPUArchState *env, int sig)
>   {
>       char buf[4];
>   
> -    if (gdbserver_fd < 0 || gdbserver_state.fd < 0) {
> +    if (!gdbserver_state.init || gdbserver_state.fd < 0) {
>           return;
>       }
>   
> @@ -3068,7 +3066,7 @@ void gdb_signalled(CPUArchState *env, int sig)
>       put_packet(buf);
>   }
>   
> -static bool gdb_accept(void)
> +static bool gdb_accept(int gdb_fd)
>   {
>       struct sockaddr_in sockaddr;
>       socklen_t len;
> @@ -3076,7 +3074,7 @@ static bool gdb_accept(void)
>   
>       for(;;) {
>           len = sizeof(sockaddr);
> -        fd = accept(gdbserver_fd, (struct sockaddr *)&sockaddr, &len);
> +        fd = accept(gdb_fd, (struct sockaddr *)&sockaddr, &len);
>           if (fd < 0 && errno != EINTR) {
>               perror("accept");
>               return false;
> @@ -3137,13 +3135,13 @@ static int gdbserver_open(int port)
>   
>   int gdbserver_start(int port)
>   {
> -    gdbserver_fd = gdbserver_open(port);
> -    if (gdbserver_fd < 0)
> +    int gdb_fd = gdbserver_open(port);
> +    if (gdb_fd < 0) {
>           return -1;
> +    }
>       /* accept connections */
> -    if (!gdb_accept()) {
> -        close(gdbserver_fd);
> -        gdbserver_fd = -1;
> +    if (!gdb_accept(gdb_fd)) {
> +        close(gdb_fd);
>           return -1;
>       }
>       return 0;
> @@ -3152,7 +3150,7 @@ int gdbserver_start(int port)
>   /* Disable gdb stub for child processes.  */
>   void gdbserver_fork(CPUState *cpu)
>   {
> -    if (gdbserver_fd < 0 || gdbserver_state.fd < 0) {
> +    if (!gdbserver_state.init || gdbserver_state.fd < 0) {
>           return;
>       }
>       close(gdbserver_state.fd);
> 

This was also already reviewed:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg697750.html

Again:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>