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

[Michael Meeks <michael ximian com>]

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;
 }