[Spice-devel,vd_agent] udscs: Clarify the udscs_read_callback() documentation

Submitted by Francois Gouget on Nov. 2, 2016, 3:36 p.m.

Details

Message ID E1c1xaJ-0003oE-Hr@amboise
State New
Headers show
Series "udscs: Clarify the udscs_read_callback() documentation" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Francois Gouget Nov. 2, 2016, 3:36 p.m.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---
 src/udscs.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/udscs.h b/src/udscs.h
index e13750c..d65630d 100644
--- a/src/udscs.h
+++ b/src/udscs.h
@@ -39,8 +39,9 @@  struct udscs_message_header {
 };
 
 /* Callbacks with this type will be called when a complete message has been
- * received. The callback may call udscs_destroy_connection, in which case
- * *connp must be made NULL (which udscs_destroy_connection takes care of).
+ * received. The callback is responsible for free()-ing the data buffer. It
+ * may call udscs_destroy_connection, in which case *connp must be made NULL
+ * (which udscs_destroy_connection takes care of).
  */
 typedef void (*udscs_read_callback)(struct udscs_connection **connp,
     struct udscs_message_header *header, uint8_t *data);

Comments

On Wed, 2016-11-02 at 16:36 +0100, Francois Gouget wrote:
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
>  src/udscs.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/udscs.h b/src/udscs.h
> index e13750c..d65630d 100644
> --- a/src/udscs.h
> +++ b/src/udscs.h
> @@ -39,8 +39,9 @@ struct udscs_message_header {
>  };
>  
>  /* Callbacks with this type will be called when a complete message
> has been
> - * received. The callback may call udscs_destroy_connection, in
> which case
> - * *connp must be made NULL (which udscs_destroy_connection takes
> care of).
> + * received. The callback is responsible for free()-ing the data
> buffer. It
> + * may call udscs_destroy_connection, in which case *connp must be
> made NULL
> + * (which udscs_destroy_connection takes care of).
>   */
>  typedef void (*udscs_read_callback)(struct udscs_connection **connp,
>      struct udscs_message_header *header, uint8_t *data);


From reading the code, it looks like the data buffer should *NOT* be
freed if udscs_destroy_connection() is called, however, since this
buffer is owned by the connection and will be freed when the connection
is destroyed. It might be worth pointing out that potential pitfall.
On Wed, 2 Nov 2016, Jonathon Jongsma wrote:
[...]
> >  /* Callbacks with this type will be called when a complete message has been
> > - * received. The callback may call udscs_destroy_connection, in which case
> > - * *connp must be made NULL (which udscs_destroy_connection takes care of).
> > + * received. The callback is responsible for free()-ing the data buffer. It
> > + * may call udscs_destroy_connection, in which case *connp must be made NULL
> > + * (which udscs_destroy_connection takes care of).
> >   */
> >  typedef void (*udscs_read_callback)(struct udscs_connection **connp,
> >      struct udscs_message_header *header, uint8_t *data);
> 
> 
> >From reading the code, it looks like the data buffer should *NOT* be
> freed if udscs_destroy_connection() is called,

Right. This API is a mess. It's also inconsistent with virtio-port.h 
where vdagent_virtio_port_read_callback() must not free the data buffer 
and must not call vdagent_virtio_port_destroy() to destroy the port!


By the way, did I miss something or does udscs_destroy_connection() 
really leak conn->data.buf if conn->read_callback is NULL?
(not that one would want read_callback to be NULL)