[g-a-devel]Re: spi_streamablecontent.c ...

[Bill Haneman <Bill Haneman Sun COM>]

Hi Michael:

spi_streamablecontent.c is definitely unfinished business.  You are
right, I left it in rather a worse state than I recalled.  Thanks for
having a look at it.  The API is currently unused since it has no
implementors at this time.  (That's expected to change when time
permits)

FWIW, the API was reviewed a long time ago, and it was publicly posted
before being committed.  In fact I think I mailed it to you :-)

The stream API was not designed to be non-blocking.  You may question
the merits of that, or whether it's appropriate for a bonobo-stream
client, but it explains why there's no completion-event API.

Thanks for the pointer to BonoboStream_seek; don't know how I missed it
the first time around.

regards,

- Bill

On Mon, 2003-09-29 at 13:00, Michael Meeks wrote:
> Hi there,
> 
> 	Just got a bug report from a user on IRC and looked at
> spi_streamablecontent.c and was somewhat horrified.
> 
> 	Leaking memory, using uninitialized CORBA_Environments, eclectic style,
> double freeing a hash key, and not checking for exceptions after
> invoking CORBA methods. I attach (an un-tested, un-compiled) patch that
> fixes the most obvious bugs;
> 
> 	It's a shame that there is (apparently) still no public code-review for
> code (and particularly API) going into at-spi; the stream API is
> somewhat incredible.
> 
> 	Regards,
> 
> 		Michael.
> 
> -- 
>  michael ximian com  <><, Pseudo Engineer, itinerant idiot
> ----
> 

> Index: cspi/spi_streamablecontent.c
> ===================================================================
> RCS file: /cvs/gnome/at-spi/cspi/spi_streamablecontent.c,v
> retrieving revision 1.7
> diff -u -p -u -r1.7 spi_streamablecontent.c
> --- cspi/spi_streamablecontent.c	19 Aug 2003 12:58:03 -0000	1.7
> +++ cspi/spi_streamablecontent.c	29 Sep 2003 12:04:41 -0000
> @@ -41,23 +41,13 @@ streams_equal_func (gconstpointer a, gco
>  }
>  
>  static void
> -stream_release (gpointer a)
> -{
> -  CORBA_Environment ev;
> -
> -  bonobo_object_release_unref (a, &ev);
> -}
> -
> -static void
>  stream_cache_item_free (gpointer a)
>  {
>    struct StreamCacheItem *cache_item = a;
> -  CORBA_Environment ev;
> -  if (cache_item) {
> -    bonobo_object_release_unref (cache_item->stream, &ev);
> -    SPI_freeString (cache_item->mimetype);
> -    g_free (cache_item);
> -  }
> +
> +  cspi_release_unref (cache_item->stream);
> +  SPI_freeString (cache_item->mimetype);
> +  g_free (cache_item);
>  }
>  
>  static GHashTable *streams = NULL;
> @@ -65,10 +55,9 @@ static GHashTable *streams = NULL;
>  GHashTable *
>  get_streams (void) 
>  {
> -  if (streams == NULL) {
> -    streams = g_hash_table_new_full (g_direct_hash, streams_equal_func, 
> -				     stream_release, stream_cache_item_free);
> -  }
> +  if (streams == NULL)
> +      streams = g_hash_table_new_full (g_direct_hash, streams_equal_func, 
> +				       NULL, stream_cache_item_free);
>    return streams;
>  }
>  
> @@ -122,6 +111,8 @@ AccessibleStreamableContent_unref (Acces
>   *       mimetypes for which the streamed content is available.
>   *
>   **/
> +
> +/* FIXME: there has to be a method that frees this array - surely */
>  char **
>  AccessibleStreamableContent_getContentTypes (AccessibleStreamableContent *obj)
>  {
> @@ -131,11 +122,11 @@ AccessibleStreamableContent_getContentTy
>  
>    mimeseq = Accessibility_StreamableContent_getContentTypes (CSPI_OBJREF (obj),
>  							     cspi_ev ());
> +  cspi_return_val_if_ev ("getContentTypes", NULL); 
>  
>    content_types = g_new0 (char *, mimeseq->_length + 1);
> -  for (i = 0; i < mimeseq->_length; ++i) {
> +  for (i = 0; i < mimeseq->_length; ++i)
>      content_types[i] = CORBA_string_dup (mimeseq->_buffer[i]);
> -  }
>    content_types [mimeseq->_length] = NULL;
>    CORBA_free (mimeseq);
>  
> @@ -162,7 +153,9 @@ AccessibleStreamableContent_open (Access
>    struct StreamCacheItem *cache;
>    stream = Accessibility_StreamableContent_getContent (CSPI_OBJREF (obj),
>  						       content_type,
> -						       cspi_ev ());    
> +						       cspi_ev ());
> +  cspi_return_val_if_ev ("getContent", FALSE); 
> +
>    if (stream != CORBA_OBJECT_NIL) {
>      cache = g_new0 (struct StreamCacheItem, 1);
>      cache->stream = stream;
> @@ -214,6 +207,8 @@ AccessibleStreamableContent_seek (Access
>  				  unsigned int seek_type)
>  {
>    /* currently Bonobo_Stream does not appear to support seek operations */
> +  /* FIXME: see; Bonobo_Stream_seek; libbonobo/idl/Bonobo_Storage.idl */
> +  /* FIXME: it's typical to return the new offset in a seek method */
>    return FALSE;
>  }
>  
> @@ -232,6 +227,8 @@ AccessibleStreamableContent_seek (Access
>   * Returns: an integer indicating the number of bytes read, or -1 on error.
>   *
>   **/
> +/* FIXME: who designed this API ? - if non-blocking, how is it going to return
> + * the number of bytes read ? - urgh */
>  SPIBoolean
>  AccessibleStreamableContent_read (AccessibleStreamableContent *obj,
>  				  void *buff,
> @@ -241,17 +238,24 @@ AccessibleStreamableContent_read (Access
>    Bonobo_Stream stream;
>    struct StreamCacheItem *cached; 
>    cached = g_hash_table_lookup (get_streams (), CSPI_OBJREF (obj));
> -  if (cached) {
> -    CORBA_long len_read;
> -    stream = cached->stream;
> -    if (stream != CORBA_OBJECT_NIL) {
> -      guint8 *mem;
> -      mem = bonobo_stream_client_read (stream, (size_t) nbytes, &len_read, cspi_ev ());
> -      if (mem) memcpy (buff, mem, len_read);	
> -      if (mem && ((nbytes == -1) || (len_read == nbytes)))
> -	return TRUE;
> +  if (cached)
> +    {
> +      CORBA_long len_read;
> +      stream = cached->stream;
> +      if (stream != CORBA_OBJECT_NIL)
> +	{
> +          guint8 *mem;
> +	  mem = bonobo_stream_client_read (stream, (size_t) nbytes, &len_read, cspi_ev ());
> +	  cspi_return_val_if_ev ("read", FALSE);
> +	  if (mem)
> +            {
> +              memcpy (buff, mem, len_read);
> +	      g_free (mem);
> +	      if ((nbytes == -1) || (len_read == nbytes))
> +                return TRUE;
> +	    }
> +	}
>      }
> -  }
>    return FALSE;
>  }
>