Re: bonobo-activation patch ... (fwd)
- From: Maciej Stachowiak <mjs noisehavoc org>
- To: Michael Meeks <michael ximian com>
- Cc: Maciej Stachowiak <mjs eazel com>, gnome-components-list gnome org
- Subject: Re: bonobo-activation patch ... (fwd)
- Date: Thu, 15 Nov 2001 15:07:41 -0800
On 11Nov2001 07:19PM (-0500), Michael Meeks wrote:
>
> Hi Maciej,
>
> Do you have time to look at this ?
>
> Regards,
>
> Michael.
>
> ---------- Forwarded message ----------
> Date: Sun, 4 Nov 2001 23:29:47 -0500 (EST)
> From: Michael Meeks <michael ximian com>
> To: Maciej Stachowiak <mjs eazel com>
> Cc: gnome-components-list gnome org
> Subject: bonobo-activation patch ...
>
>
> Hi Maciej,
>
> As suggested, added the debug_shutdown to the regression tests,
> and fixed up the resulting leaks I found:
>
> May I commit ?
>
Yep. Comments below.
>
> ? mjs
> ? bonobo-activation-0.7.0.tar.gz
> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/gnome/bonobo-activation/ChangeLog,v
> retrieving revision 1.240
> diff -u -p -u -r1.240 ChangeLog
> --- ChangeLog 2001/11/02 04:09:13 1.240
> +++ ChangeLog 2001/11/05 04:29:08
> @@ -1,3 +1,33 @@
> +2001-11-05 Michael Meeks <michael ximian com>
> +
> + * test/bonobo-activation-run-query.c (main): do a
> + _debug_shutdown + free the result coming back, beef up
> + the debugging messages.
> +
> + * server/client.c (main): do a _debug_shutdown.
> +
> + * bonobo-activation/bonobo-activation-async-callback.c
> + (bonobo_activation_async_corba_callback_new): add pre-condition,
> + store the objref in the closure, so we can unref it simply.
> + (impl_Bonobo_ActivationCallback__destroy): release the object.
> + (impl_Bonobo_ActivationCallback_report_activation_failed):
> + remove potential leak on NULL callback - no concievable
> + reason for a NULL callback anyway.
> +
> + * test/bonobo-activation-test-async.c (test_activate),
> + (test_activate_from_id): don't block in iterations in case
> + of timeout + add timeout define.
> +
> + * test/Makefile.am (TESTS/TESTS_ENVIRONMENT): use instead
> + of 'check:' hack. Add bonobo-activation-test-async.
> +
> + * test/bonobo-activation-test.c (main): use _debug_shutdown.
> + (test_empty, test_plugin): release object refs.
> +
> + * test/bonobo-activation-test-async.c (main): use
> + _debug_shutdown
> + (test_callback): release the object.
> +
> 2001-11-02 Maciej Stachowiak <mjs noisehavoc org>
>
> * NEWS, configure.in: Updated for 0.9.3.
> Index: bonobo-activation/bonobo-activation-async-callback.c
> ===================================================================
> RCS file:
> /cvs/gnome/bonobo-activation/bonobo-activation/bonobo-activation-async-callback.c,v
> retrieving revision 1.5
> diff -u -p -u -r1.5 bonobo-activation-async-callback.c
> --- bonobo-activation/bonobo-activation-async-callback.c 2001/10/17
> 07:46:53 1.5
> +++ bonobo-activation/bonobo-activation-async-callback.c 2001/11/05
> 04:29:09
> @@ -34,12 +34,11 @@
> /*** App-specific servant structures ***/
>
> typedef struct {
> -
> - POA_Bonobo_ActivationCallback servant;
> - PortableServer_POA poa;
> - BonoboActivationCallback callback;
> - gpointer user_data;
> -
> + POA_Bonobo_ActivationCallback servant;
> + PortableServer_POA poa;
> + BonoboActivationCallback callback;
> + gpointer user_data;
> + CORBA_Object objref;
> } impl_POA_Bonobo_ActivationCallback;
>
>
> @@ -63,6 +62,8 @@ impl_Bonobo_ActivationCallback__destroy
> objid = PortableServer_POA_servant_to_id (servant->poa, servant,
> ev);
> PortableServer_POA_deactivate_object (servant->poa, objid, ev);
> CORBA_free (objid);
> +
> + CORBA_Object_release (servant->objref, ev);
> }
>
> static void
> @@ -76,10 +77,6 @@ impl_Bonobo_ActivationCallback_report_ac
>
> servant = (impl_POA_Bonobo_ActivationCallback *) _servant;
>
> - if (servant->callback == NULL) {
> - return;
> - }
> -
> message = g_strconcat ("Activation failed: ", reason, NULL);
> servant->callback (CORBA_OBJECT_NIL, message,
> servant->user_data);
> g_free (message);
> @@ -101,10 +98,6 @@ impl_Bonobo_ActivationCallback_report_ac
>
> retval = CORBA_OBJECT_NIL;
>
> - if (servant->callback == NULL) {
> - return;
> - }
> -
> switch (result->res._d) {
> case Bonobo_ACTIVATION_RESULT_SHLIB:
> retval = bonobo_activation_activate_shlib_server (
> @@ -175,6 +168,8 @@ bonobo_activation_async_corba_callback_n
> PortableServer_POAManager manager;
> CORBA_ORB orb;
>
> + g_return_val_if_fail (callback != NULL, CORBA_OBJECT_NIL);
> +
> orb = bonobo_activation_orb_get ();
>
> poa = (PortableServer_POA) CORBA_ORB_resolve_initial_references (orb,
> "RootPOA", ev);
> @@ -191,6 +186,11 @@ bonobo_activation_async_corba_callback_n
> objid = PortableServer_POA_activate_object(poa, newservant, ev);
> CORBA_free(objid);
> retval = PortableServer_POA_servant_to_reference(poa, newservant, ev);
> +
> + newservant->objref = retval;
> +
> + CORBA_Object_release ((CORBA_Object) manager, ev);
> + CORBA_Object_release ((CORBA_Object) poa, ev);
>
> return retval;
> }
> Index: bonobo-activation/bonobo-activation-async.c
> ===================================================================
> RCS file:
> /cvs/gnome/bonobo-activation/bonobo-activation/bonobo-activation-async.c,v
> retrieving revision 1.8
> diff -u -p -u -r1.8 bonobo-activation-async.c
> --- bonobo-activation/bonobo-activation-async.c 2001/10/17 07:46:53
> 1.8
> +++ bonobo-activation/bonobo-activation-async.c 2001/11/05 04:29:09
> @@ -104,7 +104,6 @@ void bonobo_activation_activate_async (c
> return;
> }
>
> -
> /* make the call */
> if (ext_requirements == NULL) {
> Bonobo_ActivationContext_activate_async
> (activation_context,
No need for this gratuitous whitespace change if there are no other
changes to the file.
> Index: server/client.c
> ===================================================================
> RCS file: /cvs/gnome/bonobo-activation/server/client.c,v
> retrieving revision 1.14
> diff -u -p -u -r1.14 client.c
> --- server/client.c 2001/10/26 16:18:37 1.14
> +++ server/client.c 2001/11/05 04:29:10
> @@ -243,5 +243,9 @@ main (int argc, char *argv[])
>
> CORBA_exception_free (&ev);
>
> - return 0;
> + if (bonobo_activation_debug_shutdown ()) {
> + return 0;
> + } else {
> + return 1;
> + }
> }
Hmm, if we're going to return error values I would rather reserve it
for failing to activate, rather than potential leaks. So I don't agree
with this part of the change, unless there is some reason for it I'm
missing.
> Index: test/Makefile.am
> ===================================================================
> RCS file: /cvs/gnome/bonobo-activation/test/Makefile.am,v
> retrieving revision 1.26
> diff -u -p -u -r1.26 Makefile.am
> --- test/Makefile.am 2001/10/26 13:15:21 1.26
> +++ test/Makefile.am 2001/11/05 04:29:10
> @@ -86,5 +86,9 @@ emptydata_DATA=$(serverfiles)
>
> EXTRA_DIST=empty.idl plugin.idl $(serverfiles)
>
> -check:
> -
> BONOBO_ACTIVATION_PATH="$(top_srcdir)/test:$$BONOBO_ACTIVATION_PATH"
> PATH="$(top_builddir)/test:$$PATH"
> LD_LIBRARY_PATH="$(top_builddir)/test/$+TESTS_ENVIRONMENT = \
> +
> BONOBO_ACTIVATION_PATH="$(top_srcdir)/test:$$BONOBO_ACTIVATION_PATH" \
> + PATH="$(top_builddir)/test:$$PATH" \
> + LD_LIBRARY_PATH="$(top_builddir)/test/.libs:$$LD_LIBRARY_PATH"
> +
> +TESTS = bonobo-activation-test bonobo-activation-test-async
> Index: test/bonobo-activation-run-query.c
> ===================================================================
> RCS file:
> /cvs/gnome/bonobo-activation/test/bonobo-activation-run-query.c,v
> retrieving revision 1.5
> diff -u -p -u -r1.5 bonobo-activation-run-query.c
> --- test/bonobo-activation-run-query.c 2001/07/31 16:16:14 1.5
> +++ test/bonobo-activation-run-query.c 2001/11/05 04:29:10
> @@ -1,6 +1,7 @@
> /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8
> -*- */
> #include <stdio.h>
> #include <stdlib.h>
> +#include <string.h>
> #include "empty.h"
> #include <bonobo-activation/bonobo-activation.h>
>
> @@ -49,18 +50,36 @@ main (int argc, char *argv[])
>
> /* result = bonobo_activation_query ("iid ==
> 'OAFIID:Empty:19991025'", NULL, &ev); */
>
> - if (result == NULL) {
> - puts ("query failed");
> + if (ev._major != CORBA_NO_EXCEPTION) {
> + if (ev._major == CORBA_USER_EXCEPTION) {
> + if (!strcmp (ev._id, ex_Bonobo_GeneralError)) {
> + Bonobo_GeneralError *err =
> CORBA_exception_value (&ev);
> + printf ("An exception '%s' occured\n",
> err->description);
> + } else {
> + printf ("An exception '%s' occured\n",
> err->description);
This else clause looks like a mistake to me. What initializes err on
this code path?
> + } else {
> + printf ("An unknown user exception ('%s')
> "
> + "occured\n", ev._id);
> + }
> + } else if (ev._major == CORBA_SYSTEM_EXCEPTION) {
> + printf ("A system exception ('%s') occured\n",
> ev._id);
> + } else {
> + g_assert_not_reached ();
> + }
> + } else if (result == NULL) {
> + puts ("NULL result failed");
> } else {
> printf ("number of results: %d\n", result->_length);
>
> for (i = 0; i < result->_length; i++) {
> puts ((result->_buffer[i]).iid);
> }
> -
> + CORBA_free (result);
> }
>
> CORBA_exception_free (&ev);
>
> - return 0;
> + if (bonobo_activation_debug_shutdown ()) {
> + return 0;
> + } else {
> + return 1;
> + }
> }
We should probably add some kind of test failed message here too.
> Index: test/bonobo-activation-test-async.c
> ===================================================================
> RCS file:
> /cvs/gnome/bonobo-activation/test/bonobo-activation-test-async.c,v
> retrieving revision 1.3
> diff -u -p -u -r1.3 bonobo-activation-test-async.c
> --- test/bonobo-activation-test-async.c 2001/10/17 07:46:54 1.3
> +++ test/bonobo-activation-test-async.c 2001/11/05 04:29:11
> @@ -5,7 +5,8 @@
>
> #include <bonobo-activation/bonobo-activation.h>
>
> -#define DEBUG_TIME 0
> +#define DEBUG_TIMEOUT 2
> +#define DEBUG_TIME 1
>
> typedef struct {
> gboolean callback_called;
> @@ -13,10 +14,10 @@ typedef struct {
> } callback_data_t;
>
>
> -static
> -void test_callback (CORBA_Object activated_object,
> - const char *error_reason,
> - gpointer user_data)
> +static void
> +test_callback (CORBA_Object activated_object,
> + const char *error_reason,
> gpointer user_data)
> {
> callback_data_t *data;
>
> @@ -25,6 +26,12 @@ void test_callback (CORBA_Object activ
> if (activated_object == CORBA_OBJECT_NIL) {
> data->succeeded = FALSE;
> } else {
> + CORBA_Environment ev;
> +
> + CORBA_exception_init (&ev);
> + CORBA_Object_release (activated_object, &ev);
> + CORBA_exception_free (&ev);
> +
> data->succeeded = TRUE;
> }
>
> @@ -53,9 +60,9 @@ test_activate (char *requirements)
> #endif
>
> while (data.callback_called == FALSE) {
> - g_main_iteration (TRUE);
> + g_main_iteration (FALSE);
> #if DEBUG_TIME
> - if (time (NULL) > (beg_time + 10)) {
> + if (time (NULL) > (beg_time + DEBUG_TIMEOUT)) {
> return -1;
> }
> #endif
> @@ -90,9 +97,9 @@ test_activate_from_id (char *aid)
> #endif
>
> while (data.callback_called == FALSE) {
> - g_main_iteration (TRUE);
> + g_main_iteration (FALSE);
> #if DEBUG_TIME
> - if (time (NULL) > (beg_time + 10)) {
> + if (time (NULL) > (beg_time + DEBUG_TIMEOUT)) {
> return -1;
> }
> #endif
> @@ -167,7 +174,11 @@ main (int argc, char *argv[])
> return 1;
> }
>
> - return 0;
> + if (bonobo_activation_debug_shutdown ()) {
> + return 0;
> + } else {
> + return 1;
> + }
> }
Ditto previous remark.
>
>
> Index: test/bonobo-activation-test.c
> ===================================================================
> RCS file: /cvs/gnome/bonobo-activation/test/bonobo-activation-test.c,v
> retrieving revision 1.23
> diff -u -p -u -r1.23 bonobo-activation-test.c
> --- test/bonobo-activation-test.c 2001/10/26 13:15:21 1.23
> +++ test/bonobo-activation-test.c 2001/11/05 04:29:11
> @@ -85,6 +85,7 @@ test_plugin (CORBA_Object obj, CORBA_Env
> return 0;
> } else {
> fprintf (stderr, "Test %s succeeded\n", type);
> + CORBA_Object_release (obj, ev);
> return 1;
> }
> }
> @@ -100,6 +101,7 @@ test_empty (CORBA_Object obj, CORBA_Envi
> return 0;
> } else {
> fprintf (stderr, "Test %s succeeded\n", type);
> + CORBA_Object_release (obj, ev);
> return 1;
> }
> }
> @@ -244,7 +246,11 @@ main (int argc, char *argv[])
> CORBA_exception_free (&ev);
>
> if (passed == TOTAL_TEST_SCORE) {
> - return 0;
> + if (bonobo_activation_debug_shutdown ()) {
> + return 0;
> + } else {
> + return 1;
> + }
And once again.
> } else {
> return 1;
Any reason not to do the memory test if other tests fail>
> }
>
> It seems this version has slightly screwed wrapping, the previous
> version I sent last week didn't.
I can read it, no big deal.
Sorry this took so long.
- Maciej
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
