GNOME.org
 

Re: Oaf patch ...

On Tuesday, July 17, 2001, at 10:46  AM, Michael Meeks wrote:


Index: liboaf/oaf-activate.c
===================================================================
RCS file: /cvs/gnome/oaf/liboaf/oaf-activate.c,v
retrieving revision 1.22
diff -u -r1.22 oaf-activate.c
--- liboaf/oaf-activate.c	2001/07/16 18:22:48	1.22
+++ liboaf/oaf-activate.c	2001/07/17 17:28:33
@@ -341,7 +341,11 @@
  */
 CORBA_Object oaf_name_service_get (CORBA_Environment * ev)
 {
+        CORBA_Object ret;
+	ret = oaf_activate_from_id (
+ "OAFIID:oaf_naming_service:7e2b90ef-eaf0-4239-bb7c-812606fcd80d", 
+                0, NULL, ev);
+        fprintf (stderr, "Oaf name service get = '%p'", ret);
- return oaf_activate_from_id ("OAFIID:oaf_naming_service:7e2b90ef-eaf0-4239-bb7c-812606fcd80d", 
-				     0, NULL, ev);
+        return ret;
 }
I personally don't like programs that spew messages to g_message and stderr as part of normal operation, although I know the messages can be very useful for debugging. I'd strongly request that this be controlled by an environment variable or inside #if OAF_DEBUG or something like that. I' d also suggest a "\n" at the end of the message string if you are going to use fprintf instead of g_message. 


Index: liboaf/oaf-fork-server.c
===================================================================
RCS file: /cvs/gnome/oaf/liboaf/oaf-fork-server.c,v
retrieving revision 1.3
diff -u -r1.3 oaf-fork-server.c
--- liboaf/oaf-fork-server.c	2001/07/16 18:22:48	1.3
+++ liboaf/oaf-fork-server.c	2001/07/17 17:28:33
@@ -27,7 +27,7 @@
 #define _GNU_SOURCE 1
 #endif
 #include <string.h>
-
+#include <config.h>
I normally put the include of <config.h> before anything else -- before other includes or defines. 


@@ -157,7 +157,6 @@
                 sigprocmask (SIG_SETMASK, &omask, NULL);
 		errval = OAF_GeneralError__alloc ();
errval->description = CORBA_string_dup (_("Couldn't fork a new process")); 
-
 		CORBA_exception_set (ev, CORBA_USER_EXCEPTION,
 				     ex_OAF_GeneralError, errval);
 		return CORBA_OBJECT_NIL;
Checking in this kind of gratuitous white space change potentially makes future merges herder without paying back much. 


@@ -221,8 +220,10 @@
 				retval = CORBA_OBJECT_NIL;
 #ifdef OAF_DEBUG
 			if (ai.do_srv_output)
-				g_message ("Did string_to_object on %s",
-					   ai.iorbuf);
+				g_message ("Did string_to_object on %s = '%p' (%s)",
+					   ai.iorbuf, retval,
+ ev->_major == CORBA_NO_EXCEPTION? 
+                                           "no-exception" : ev->_id);
 #endif
 		} else {
 			OAF_GeneralError *errval;
This looks good. Just out of curiosity, is ev->_id guaranteed to be a human-readable string, or is that just "the usual convention"? Can it be NULL even though ev->_major is set? 


Index: liboaf/oaf-registration.c
===================================================================
RCS file: /cvs/gnome/oaf/liboaf/oaf-registration.c,v
retrieving revision 1.29
diff -u -r1.29 oaf-registration.c
--- liboaf/oaf-registration.c	2001/07/16 18:22:48	1.29
+++ liboaf/oaf-registration.c	2001/07/17 17:28:33
@@ -469,7 +469,6 @@
 			break;
 	}

-	lock_fd = open (fn, O_CREAT | O_RDONLY, 0700);
 	fcntl (lock_fd, F_SETFD, FD_CLOEXEC);

 	if (lock_fd >= 0) {


Good catch! Seems pretty broken the way it is.


Index: oafd/main.c
===================================================================
RCS file: /cvs/gnome/oaf/oafd/main.c,v
retrieving revision 1.30
diff -u -r1.30 main.c
--- oafd/main.c	2001/07/13 00:50:30	1.30
+++ oafd/main.c	2001/07/17 17:28:33
@@ -97,7 +97,7 @@
 	FILE *fh;
 	struct sigaction sa;
         const char *oaf_debug_output;
-        int dev_null_fd;
+        int dev_null_fd = -1;

 	if (chdir ("/")) {
 		g_print ("Couldn't chdir() to '/' (why ?!!). Exiting.\n");
I think dev_null_fd is already set to -1 later on in the function. At the time you started work on this change, perhaps it wasn't, but I added that code in my "reduce warnings in oaf" check-in. So you need to either not do this, or remove the "dev_null_fd = -1;" below. 


@@ -151,6 +151,7 @@
         }

 	orb = oaf_init (argc, argv);
+
 	ml = g_main_new (FALSE);

 	root_poa = (PortableServer_POA)
@@ -217,6 +218,7 @@

 		g_string_free (real_od_source_dir, TRUE);
 	}
+
 	if (server_ac) {
 		primary_server = ac =
 			OAF_ActivationContext_create (root_poa, &ev);
More of these white space tweaks. They are OK if they make things substantially better, but usually a bad idea to commit if merging across branches is going to be done later. 


@@ -232,7 +234,7 @@
 	if (fh) {
 		fprintf (fh, "%s\n", ior);
 		fclose (fh);
-		if(ior_fd <= 2)
+		if(ior_fd <= 2 && dev_null_fd > 0)
 		  dup2 (dev_null_fd, ior_fd);
 	} else {
 		printf ("%s\n", ior);
I think you need to make sure dev_null_fd is opened in this case, rather than just not doing the dup2 if it's not already open. 


Index: oafd/od-corba.c
===================================================================
RCS file: /cvs/gnome/oaf/oafd/od-corba.c,v
retrieving revision 1.31
diff -u -r1.31 od-corba.c
--- oafd/od-corba.c	2001/05/15 19:53:15	1.31
+++ oafd/od-corba.c	2001/07/17 17:28:33
@@ -163,10 +163,14 @@
 			OAF_Property *prop =
 				&(od->attr_servers._buffer[i].
 				  props._buffer[j]);
+ if (strstr (prop->name, "-")) /* Translated, likely to + be annoying garbage value */ 
+                                continue;
+
Would be best to write out a newline here since we've already printed a partial line at this point. 


 			g_print ("    %s = ", prop->name);
 			switch (prop->v._d) {
 			case OAF_P_STRING:
-				g_print ("\"%s\"\n", prop->v._u.value_string);
+ g_print ("\"%s\"\n", prop->v._u.value_string); 
 				break;
 			case OAF_P_NUMBER:
 				g_print ("%f\n", prop->v._u.value_number);


Another white space tweak that need not go in.


Index: test/oaf-run-query.c
===================================================================
RCS file: /cvs/gnome/oaf/test/oaf-run-query.c,v
retrieving revision 1.4
diff -u -r1.4 oaf-run-query.c
--- test/oaf-run-query.c	2000/05/27 14:47:10	1.4
+++ test/oaf-run-query.c	2001/07/17 17:28:33
@@ -11,12 +11,18 @@
 	CORBA_Environment ev;
 	char *query;
 	char **sort_criteria;
-	int i;
+	int i, j;

-	CORBA_exception_init (&ev);
 	oaf_init (argc, argv);
+	CORBA_exception_init (&ev);
Seems arbitrary to reorder the lines like this. No need to make such a change unless there's a substantial benefit to outweigh the potential minor merge headache. 


 	sort_criteria = NULL;
+
+        for (j = 0; j < 5; j++) {
+                oaf_name_service_get (&ev);
+                if (ev._major == CORBA_NO_EXCEPTION)
+                        g_warning ("Error getting name service '%s'", ev.
_id);
+        }

 	if (argc > 1) {
 		query = argv[1];
If you're going to have this loop to get the name service 5 times, you should at least write a comment and explain why you're doing it. 

    -- Darin
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]