Re: [PATCHv5 3/4] gdb/python: add gdb.RemoteTargetConnection.send_packet

[Simon Marchi <simon.marchi@polymtl.ca>]

Just some minor comments (one about a possible bug), but otherwise this
looks fine to me.

> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index 2234bbe1891..0fd31ca6cb3 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -5989,9 +5989,9 @@
>  Examples of different connection types are @samp{native} and
>  @samp{remote}.  @xref{Inferiors Connections and Programs}.
>  
> -@value{GDBN} uses the @code{gdb.TargetConnection} object type to
> -represent a connection in Python code.  To get a list of all
> -connections use @code{gdb.connections}
> +Connections in @value{GDBN} are represented as instances of
> +@code{gdb.TargetConnection}, or as one of its sub-classes.  To get a
> +list of all connections use @code{gdb.connections}
>  (@pxref{gdbpy_connections,,gdb.connections}).
>  
>  To get the connection for a single @code{gdb.Inferior} read its
> @@ -6044,6 +6044,40 @@
>  to the remote target.
>  @end defvar
>  
> +The @code{gdb.RemoteTargetConnection} class is a sub-class of
> +@code{gdb.TargetConnection}, and is used to represent @samp{remote}
> +and @samp{extended-remote} connections.  In addition to the attributes
> +and methods available from the @code{gdb.TargetConnection} base class,
> +a @code{gdb.RemoteTargetConnection} has the following method:

In your opinion, would it be ok (backwards-compatible-wise) to introduce
a gdb.ExtendedRemoteTargetConnection class in the future, should we ever need
that?  The only case that I think of that would break is if somebody
does:

  type(my_target) is gdb.RemoteTargetConnection

where my_target is an extended-remote connection.  That expression would
return True today, and would return False after adding
gdb.ExtendedRemoteTargetConnection.  But if that could would use
isinstance, then it would be fine.

> @@ -284,9 +295,111 @@ gdbpy_initialize_connection (void)
>  			      (PyObject *) &connection_object_type) < 0)
>      return -1;
>  
> +  if (PyType_Ready (&remote_connection_object_type) < 0)
> +    return -1;
> +
> +  if (gdb_pymodule_addobject (gdb_module, "RemoteTargetConnection",
> +			      (PyObject *) &remote_connection_object_type) < 0)
> +    return -1;
> +
>    return 0;
>  }
>  
> +/* Set of callbacks used to implement gdb.send_packet.  */
> +
> +struct py_send_packet_callbacks : public send_remote_packet_callbacks
> +{
> +  /* Constructor, initialise the result to None.  */
> +
> +  py_send_packet_callbacks ()
> +    : m_result (Py_None)
> +  { /* Nothing.  */ }

I think you might need to incref Py_None here.  the gdbpy_ref will
decref on destruction or when its value changes, but you never incref
it.

> +
> +  /* There's nothing to do when the packet is sent.  */
> +
> +  void sending (const char *args) override
> +  { /* Nothing.  */ }
> +
> +  /* When the result is returned create a Python string and assign this
> +     into the result member variable.  */
> +
> +  void received (const gdb::char_vector &buf) override
> +  {
> +    /* m_result is initialized to None, leave it untouched unless we got
> +       back a useful result.  */
> +    if (buf.data ()[0] != '\0')
> +      {
> +	PyObject *result;
> +
> +#ifdef IS_PY3K
> +	result = Py_BuildValue("y#", buf.data (), strlen (buf.data ()));

Missing space.

Simon