Re: Next oaf patch...

[Maciej Stachowiak <mjs eazel com>]

Michael Meeks <michael@helixcode.com> writes:

> Ok,
> 
> 	This next patch fixes a segfault in oafd where it was throwing
> exceptions that it hadn't declared in  oaf.idl, the offender being
> activate_from_id. It also adds a more systematic test-client that catches
> this regression and also checks to see whether the daemon died recently [
> although the CORBA_Object pointer compare == same object seems rather
> flawed ] it can't report false positives AFAICS.
> 

Looks mostly good, some comments below on minor issues.

> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/gnome/oaf/ChangeLog,v
> retrieving revision 1.17
> diff -u -r1.17 ChangeLog
> --- ChangeLog	2000/05/27 14:47:09	1.17
> +++ ChangeLog	2000/05/27 15:13:14
> @@ -1,16 +1,27 @@
>  2000-05-26  Michael Meeks  <michael@helixcode.com>
>  
> +	* test/empty.oafinfo: Add bogus entry with deliberately broken
> +	factory location.
> +
> +	* idl/oaf.idl: Fix missing exceptions causing oafd to segfault.
> +
> +	* test/oaf-test-client.c (empty_test): expand.
> +	(test_object): add. (test_oafd): see if we crashed the daemon
> +	(main): update to add test for fix.
> +
> +2000-05-26  Michael Meeks  <michael@helixcode.com>
> +	
>  	* test/oaf-run-query.c (main): sort comments.
> - 
> +
>  	* test/Makefile.am: Stupid cut and paste error fixed, re-instating the
> - 	test client, doh.
> - 
> +	test client, doh.
> +
>  	* test/oaf-test-client.c (test_empty): split.
> - 	(main): expand.
> - 
> +	(main): expand.
> +
>  	* oafd/ac-query-expr.c (qexp_sort): cast sort_compare to right type,
> - 	include qsort_ex.h
> - 
> +	include qsort_ex.h
> +
>  2000-05-24  Darin Adler  <darin@eazel.com>
>  
>  	* docs/standard-attributes.txt: Updated documentation of some
> Index: idl/oaf.idl
> ===================================================================
> RCS file: /cvs/gnome/oaf/idl/oaf.idl,v
> retrieving revision 1.14
> diff -u -r1.14 oaf.idl
> --- idl/oaf.idl	2000/05/07 18:45:32	1.14
> +++ idl/oaf.idl	2000/05/27 15:13:14
> @@ -124,7 +124,7 @@
>                  ActivationResult activate (in string requirements,
>                                             in GNOME::stringlist selection_order,
>                                             in ActivationFlags flags)
> -                        raises (ParseFailed)
> +                        raises (ParseFailed, GeneralError)
>                          context ("username", "hostname", "domain");
>  
>                  readonly attribute ServerInfoList servers;
> @@ -134,6 +134,7 @@
>                          context ("username", "hostname", "domain");
>  
>                  ActivationResult activate_from_id (in ActivationID aid, in ActivationFlags flags)
> +                        raises (ParseFailed, GeneralError)
>                          context ("username", "hostname", "domain");
>          };
>  };
> Index: oafd/ac-corba.c
> ===================================================================
> RCS file: /cvs/gnome/oaf/oafd/ac-corba.c,v
> retrieving revision 1.19
> diff -u -r1.19 ac-corba.c
> --- oafd/ac-corba.c	2000/05/27 14:47:10	1.19
> +++ oafd/ac-corba.c	2000/05/27 15:13:14
> @@ -537,14 +537,12 @@
>  
>  	servant->refs++;
>  
> -	items =
> -		oaf_alloca (servant->total_servers *
> +	items = oaf_alloca (servant->total_servers *
>  			    sizeof (OAF_ServerInfo *));
>  	ac_query_run (servant, requirements, selection_order, ctx, items, ev);
>  
> -	if (ev->_major != CORBA_NO_EXCEPTION) {
> +	if (ev->_major != CORBA_NO_EXCEPTION)
>  		goto out;
> -	}

Please don't do that, I like to have even single-line if consequents
in curly braces.

>  	retval = OAF_ActivationResult__alloc ();
>  	retval->res._d = OAF_RESULT_NONE;
> @@ -915,7 +913,6 @@
>  		CORBA_free (retval);
>  		retval = NULL;
>  	}
> -
>  
>  	return retval;
>  }
>
> Index: test/empty.oafinfo
> ===================================================================
> RCS file: /cvs/gnome/oaf/test/empty.oafinfo,v
> retrieving revision 1.3
> diff -u -r1.3 empty.oafinfo
> --- test/empty.oafinfo	2000/05/27 14:47:10	1.3
> +++ test/empty.oafinfo	2000/05/27 15:13:15
> @@ -1,5 +1,12 @@
> +<oaf_info>
> +
>  <oaf_server iid="OAFIID:Empty:19991025" type="exe" location="./oaf-empty-server">
>  <oaf_attribute name="repo_ids" type="stringv">
>  	<item value="IDL:Empty:1.0"/>
>  </oaf_attribute>
>  </oaf_server>
> +
> +<oaf_server iid="OAFIID:Bogus:20000526" type="factory" location="OAFIID:Empty:19991025-Deliberate-Typo">
> +</oaf_server>
> +
> +</oaf_info>
> \ No newline at end of file

Please, um, add a newline at the end of the file. :-) Also, it might
be helpful to put the bogus iid data in a separate oafinfo file
(`broken.oafinfo' ?) just to distinguish the working stuff from the
non-working stuff.

> Index: test/oaf-test-client.c
> ===================================================================
> RCS file: /cvs/gnome/oaf/test/oaf-test-client.c,v
> retrieving revision 1.5
> diff -u -r1.5 oaf-test-client.c
> --- test/oaf-test-client.c	2000/05/27 14:47:10	1.5
> +++ test/oaf-test-client.c	2000/05/27 15:13:15
> @@ -4,25 +4,78 @@
>  #include <stdlib.h>
>  #include "empty.h"
>  
> -void
> -test_empty (CORBA_Object obj, CORBA_Environment *ev, const char *type)
> +CORBA_Object name_service = CORBA_OBJECT_NIL;
> +
> +char *
> +oaf_exception_id (CORBA_Environment *ev)
> +{
> +        if (ev->_major == CORBA_USER_EXCEPTION) {
> +                if (!strcmp (ev->_repo_id, "IDL:OAF/GeneralError:1.0")) {
> +                        OAF_GeneralError *err = ev->_params;
> +                        
> +                        if (!err || !err->description)
> +                                return "No general exception error message";
> +                        else
> +                                return err->description;
> +                } else
> +                        return ev->_repo_id;
> +        } else
> +                return CORBA_exception_id (ev);
> +}
> +
> +gboolean
> +test_oafd (CORBA_Environment *ev, const char *type)
> +{
> +        CORBA_Object ns;
> +
> +        ns = oaf_name_service_get (ev);
> +        if (ev->_major != CORBA_NO_EXCEPTION) {
> +                g_warning ("Exception '%s' finding oafd %s",
> +                           oaf_exception_id (ev), type);
> +                return FALSE;
> +        }
> +
> +        if (name_service != CORBA_OBJECT_NIL &&
> +            name_service != ns) {
> +                g_warning ("oafd crashed %s", type);
> +                return FALSE;
> +        }
> +
> +        name_service = ns;
> +
> +        return TRUE;
> +}
> +
> +gboolean
> +test_object (CORBA_Object obj, CORBA_Environment *ev, const char *type)
>  {
>  	if (CORBA_Object_is_nil (obj, ev)) {
>  		g_warning ("Activation %s failed!", type);
>  
>  	} else if (ev->_major != CORBA_NO_EXCEPTION) {
>  		g_warning ("Activation %s failed: %s\n", type,
> -			   CORBA_exception_id (ev));
> -	} else {
> -		Empty_doNothing (obj, ev);
> -		if (ev->_major != CORBA_NO_EXCEPTION)
> -			g_warning ("Call failed: %s\n",
> -				   CORBA_exception_id (ev));
> -                else
> -                        fprintf (stderr, "Test %s succeeded\n", type);
> -	}
> +			   oaf_exception_id (ev));
> +	} else
> +                return TRUE;
> +
> +        if (!test_oafd (ev, type))
> +                return FALSE;
> +
> +        return FALSE;
>  }
>  
> +void
> +test_empty (CORBA_Object obj, CORBA_Environment *ev, const char *type)
> +{
> +        Empty_doNothing (obj, ev);
> +
> +        if (ev->_major != CORBA_NO_EXCEPTION)
> +                g_warning ("Call failed: %s\n",
> +                           oaf_exception_id (ev));
> +        else
> +                fprintf (stderr, "Test %s succeeded\n", type);
> +}
> +
>  int
>  main (int argc, char *argv[])
>  {
> @@ -33,18 +86,36 @@
>  	oaf_init (argc, argv);
>  
>  /*      putenv("OAF_BARRIER_INIT=1"); */
> +
>  	obj = oaf_activate ("repo_ids.has('IDL:Empty:1.0')", NULL, 0, NULL,
>                              &ev);
> +        if (test_object (obj, &ev, "by query"))
> +                test_empty (obj, &ev, "by query");
>  
> -        test_empty (obj, &ev, "by query");
>  
>  	obj = oaf_activate_from_id ("OAFIID:Empty:19991025", 0, NULL, &ev);
> +        if (test_object (obj, &ev, "from id"))
> +                test_empty (obj, &ev, "from id");
>  
> -        test_empty (obj, &ev, "from id");
>  
>  	obj = oaf_activate_from_id ("OAFAID:[OAFIID:Empty:19991025]", 0, NULL, &ev);
> +        if (test_object (obj, &ev, "from aid"))
> +                test_empty (obj, &ev, "from aid");
> +
>  
> -        test_empty (obj, &ev, "from aid");
> +        fprintf (stderr, "Broken link test ");
> +        obj = oaf_activate_from_id ("OAFIID:Bogus:20000526", 0, NULL, &ev);
> +        if (obj || ev._major == CORBA_NO_EXCEPTION)
> +                fprintf (stderr, "failed 1");
> +        else {
> +                fprintf (stderr, "passed 1");
> +                CORBA_exception_free (&ev);
> +        }
> +        if (test_oafd (&ev, "with broken factory link"))
> +                fprintf (stderr, ", passed 2");
> +        else
> +                fprintf (stderr, ", failed 2");
> +        fprintf (stderr, "\n");
>  
>  	CORBA_exception_free (&ev);
>