Re: [PATCH] gdbserver: Introduce write_error_msg

[Luis Machado <lgustavo@codesourcery.com>]

On 11/23/2016 09:35 AM, Pedro Alves wrote:
> Instead of writing an error message to (gdbserver's) stderr and then
> returning back "E01" to GDB, send the error message to GDB directly
> using the E.MSG format, so the user can potentially see it.  Several
> places in the code already do this manually.  This patch adds a new
> function (write_error_msg) that abstracts out the "E." prefix detail.
>
> gdb/gdbserver/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
>
> 	* remote-utils.c (write_error_msg): New function.
> 	* remote-utils.h (write_error_msg): Declare.
> 	* server.c (handle_btrace_general_set)
> 	(handle_btrace_conf_general_set, handle_general_set)
> 	(handle_qxfer_btrace, handle_qxfer_btrace_conf, resume)
> 	(handle_v_requests): Use write_error_msg.
> ---
>  gdb/gdbserver/remote-utils.c | 15 +++++++++
>  gdb/gdbserver/remote-utils.h |  7 +++++
>  gdb/gdbserver/server.c       | 73 +++++++++++++++++++++-----------------------
>  3 files changed, 57 insertions(+), 38 deletions(-)
>
> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
> index 3212ec9..b6c521b 100644
> --- a/gdb/gdbserver/remote-utils.c
> +++ b/gdb/gdbserver/remote-utils.c
> @@ -1083,6 +1083,21 @@ write_enn (char *buf)
>    buf[3] = '\0';
>  }
>
> +/* See remote-utils.h.  */
> +
> +void
> +write_error_msg (char *buf, const char *fmt, ...)
> +{
> +  va_list ap;
> +
> +  va_start (ap, fmt);
> +
> +  buf[0] = 'E';
> +  buf[1] = '.';
> +  vsprintf (own_buf + 2, fmt, ap);
> +  va_end (ap);
> +}
> +
>  #endif
>
>  #ifndef IN_PROCESS_AGENT
> diff --git a/gdb/gdbserver/remote-utils.h b/gdb/gdbserver/remote-utils.h
> index 2ddf590..9f7a587 100644
> --- a/gdb/gdbserver/remote-utils.h
> +++ b/gdb/gdbserver/remote-utils.h
> @@ -40,6 +40,13 @@ void remote_open (char *name);
>  void remote_close (void);
>  void write_ok (char *buf);
>  void write_enn (char *buf);
> +
> +/* Write an RSP error message in BUF, using the "E.MSG" format.  The
> +   error message is constructed using a printf style argument
> +   list.  */
> +void write_error_msg (char *buf, const char *fmt, ...)
> +  ATTRIBUTE_PRINTF (2, 3);
> +
>  void initialize_async_io (void);
>  void enable_async_io (void);
>  void disable_async_io (void);
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index ef8dd03..006d768 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -500,14 +500,14 @@ handle_btrace_general_set (char *own_buf)
>    if (ptid_equal (general_thread, null_ptid)
>        || ptid_equal (general_thread, minus_one_ptid))
>      {
> -      strcpy (own_buf, "E.Must select a single thread.");
> +      write_error_msg (own_buf, "Must select a single thread.");
>        return -1;
>      }
>
>    thread = find_thread_ptid (general_thread);
>    if (thread == NULL)
>      {
> -      strcpy (own_buf, "E.No such thread.");
> +      write_error_msg (own_buf, "No such thread.");
>        return -1;
>      }
>
> @@ -546,14 +546,14 @@ handle_btrace_conf_general_set (char *own_buf)
>    if (ptid_equal (general_thread, null_ptid)
>        || ptid_equal (general_thread, minus_one_ptid))
>      {
> -      strcpy (own_buf, "E.Must select a single thread.");
> +      write_error_msg (own_buf, "Must select a single thread.");
>        return -1;
>      }
>
>    thread = find_thread_ptid (general_thread);
>    if (thread == NULL)
>      {
> -      strcpy (own_buf, "E.No such thread.");
> +      write_error_msg (own_buf, "No such thread.");
>        return -1;
>      }
>
> @@ -566,7 +566,7 @@ handle_btrace_conf_general_set (char *own_buf)
>        size = strtoul (op + strlen ("bts:size="), &endp, 16);
>        if (endp == NULL || *endp != 0 || errno != 0 || size > UINT_MAX)
>  	{
> -	  strcpy (own_buf, "E.Bad size value.");
> +	  write_error_msg (own_buf, "Bad size value.");
>  	  return -1;
>  	}
>
> @@ -581,7 +581,7 @@ handle_btrace_conf_general_set (char *own_buf)
>        size = strtoul (op + strlen ("pt:size="), &endp, 16);
>        if (endp == NULL || *endp != 0 || errno != 0 || size > UINT_MAX)
>  	{
> -	  strcpy (own_buf, "E.Bad size value.");
> +	  write_error_msg (own_buf, "Bad size value.");
>  	  return -1;
>  	}
>
> @@ -589,7 +589,7 @@ handle_btrace_conf_general_set (char *own_buf)
>      }
>    else
>      {
> -      strcpy (own_buf, "E.Bad Qbtrace configuration option.");
> +      write_error_msg (own_buf, "Bad Qbtrace configuration option.");
>        return -1;
>      }
>
> @@ -673,9 +673,10 @@ handle_general_set (char *own_buf)
>  	enabled = 1;
>        else
>  	{
> -	  fprintf (stderr, "Unknown catch-syscalls mode requested: %s\n",
> -		   own_buf);
> -	  write_enn (own_buf);
> +	  /* Must make a copy of P, since P overlaps with OWN_BUF.  */
> +	  write_error_msg (own_buf,
> +			   "Unknown catch-syscalls mode requested: %s\n",
> +			   std::string (p).c_str ());
>  	  return;
>  	}
>
> @@ -727,19 +728,18 @@ handle_general_set (char *own_buf)
>  	req = 1;
>        else
>  	{
> -	  /* We don't know what this mode is, so complain to
> -	     GDB.  */
> -	  fprintf (stderr, "Unknown non-stop mode requested: %s\n",
> -		   own_buf);
> -	  write_enn (own_buf);
> +	  /* We don't know what this mode is, so complain to GDB.
> +	     Must make a copy of MODE, since MODE overlaps with
> +	     OWN_BUF.  */
> +	  write_error_msg (own_buf, "Unknown non-stop mode requested: %s\n",
> +			   std::string (mode).c_str ());
>  	  return;
>  	}
>
>        req_str = req ? "non-stop" : "all-stop";
>        if (start_non_stop (req) != 0)
>  	{
> -	  fprintf (stderr, "Setting %s mode failed\n", req_str);
> -	  write_enn (own_buf);
> +	  write_error_msg (own_buf, "Setting %s mode failed\n", req_str);
>  	  return;
>  	}
>
> @@ -787,7 +787,7 @@ handle_general_set (char *own_buf)
>        else
>  	{
>  	  /* We don't know what this value is, so complain to GDB.  */
> -	  sprintf (own_buf, "E.Unknown QAgent value");
> +	  write_error_msg (own_buf, "Unknown QAgent value");
>  	  return;
>  	}
>
> @@ -816,12 +816,12 @@ handle_general_set (char *own_buf)
>  	req = TRIBOOL_TRUE;
>        else
>  	{
> -	  char *mode_copy = xstrdup (mode);
> -
> -	  /* We don't know what this mode is, so complain to GDB.  */
> -	  sprintf (own_buf, "E.Unknown thread-events mode requested: %s\n",
> -		   mode_copy);
> -	  xfree (mode_copy);
> +	  /* We don't know what this mode is, so complain to GDB.
> +	     (Must make a copy of MODE, since MODE overlaps with
> +	     OWN_BUF.)  */
> +	  write_error_msg (own_buf,
> +			   "Unknown thread-events mode requested: %s\n",
> +			   std::string (mode).c_str ());
>  	  return;
>  	}
>
> @@ -1735,20 +1735,20 @@ handle_qxfer_btrace (const char *annex,
>    if (ptid_equal (general_thread, null_ptid)
>        || ptid_equal (general_thread, minus_one_ptid))
>      {
> -      strcpy (own_buf, "E.Must select a single thread.");
> +      write_error_msg (own_buf, "Must select a single thread.");
>        return -3;
>      }
>
>    thread = find_thread_ptid (general_thread);
>    if (thread == NULL)
>      {
> -      strcpy (own_buf, "E.No such thread.");
> +      write_error_msg (own_buf, "No such thread.");
>        return -3;
>      }
>
>    if (thread->btrace == NULL)
>      {
> -      strcpy (own_buf, "E.Btrace not enabled.");
> +      write_error_msg (own_buf, "Btrace not enabled.");
>        return -3;
>      }
>
> @@ -1760,7 +1760,7 @@ handle_qxfer_btrace (const char *annex,
>      type = BTRACE_READ_DELTA;
>    else
>      {
> -      strcpy (own_buf, "E.Bad annex.");
> +      write_error_msg (own_buf, "Bad annex.");
>        return -3;
>      }
>
> @@ -1809,20 +1809,20 @@ handle_qxfer_btrace_conf (const char *annex,
>    if (ptid_equal (general_thread, null_ptid)
>        || ptid_equal (general_thread, minus_one_ptid))
>      {
> -      strcpy (own_buf, "E.Must select a single thread.");
> +      write_error_msg (own_buf, "Must select a single thread.");
>        return -3;
>      }
>
>    thread = find_thread_ptid (general_thread);
>    if (thread == NULL)
>      {
> -      strcpy (own_buf, "E.No such thread.");
> +      write_error_msg (own_buf, "No such thread.");
>        return -3;
>      }
>
>    if (thread->btrace == NULL)
>      {
> -      strcpy (own_buf, "E.Btrace not enabled.");
> +      write_error_msg (own_buf, "Btrace not enabled.");
>        return -3;
>      }
>
> @@ -2790,7 +2790,7 @@ resume (struct thread_resume *actions, size_t num_actions)
>  	{
>  	  /* The client does not support this stop reply.  At least
>  	     return error.  */
> -	  sprintf (own_buf, "E.No unwaited-for children left.");
> +	  write_error_msg (own_buf, "No unwaited-for children left.");
>  	  disable_async_io ();
>  	  return;
>  	}
> @@ -3016,8 +3016,7 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
>      {
>        if ((!extended_protocol || !multi_process) && target_running ())
>  	{
> -	  fprintf (stderr, "Already debugging a process\n");
> -	  write_enn (own_buf);
> +	  write_error_msg (own_buf, "Already debugging a process");
>  	  return;
>  	}
>        handle_v_attach (own_buf);
> @@ -3028,8 +3027,7 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
>      {
>        if ((!extended_protocol || !multi_process) && target_running ())
>  	{
> -	  fprintf (stderr, "Already debugging a process\n");
> -	  write_enn (own_buf);
> +	  write_error_msg (own_buf, "Already debugging a process");
>  	  return;
>  	}
>        handle_v_run (own_buf);
> @@ -3040,8 +3038,7 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
>      {
>        if (!target_running ())
>  	{
> -	  fprintf (stderr, "No process to kill\n");
> -	  write_enn (own_buf);
> +	  write_error_msg (own_buf, "No process to kill");
>  	  return;
>  	}
>        handle_v_kill (own_buf);
>

This looks OK to me, though the duplicate messages caught my attention. 
We could do something about them (having a predefined string that gets 
reused).

Not sure if we care about translations of such messages. Anyway, just a 
suggestion.