[Openais] [patch] saEvtDispatch seg fault

Russell Bryant russell at digium.com
Tue Aug 14 07:22:53 PDT 2007


Steven Dake wrote:
> The initialize function should initialize any data areas needed for
> operation for APIs that cannot return SA_AIS_ERR_NO_MEMORY.
> =

> If you want to modify your patch to do that it would be much appreciated
> - or I should get to it at some point in the future after my current
> plate of bugs clears up.

I have attached a patch that addresses your recommendations.  It does the f=
ollowing:

* Moves the 1 MB dispatch_data that was allocated on the stack in evtDispat=
ch, =

to be a member of the event_instance struct.

* Tweaks the locking in evtDispatch such that the dispatch_mutex also ensur=
es =

that only one thread is using the dispatch_data buffer.
  - Unfortunately, this means holding the mutex longer than before.  The on=
ly =

way I see to improve this would be to use a thread local buffer here, inste=
ad. =

However, that would put us back to doing the allocation in evtDispatch inst=
ead =

of evtInitialize.

The patch was made against the whitetank branch, but will also apply to tru=
nk.

-- =

Russell Bryant
Software Engineer
Digium, Inc.
-------------- next part --------------
Index: lib/evt.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- lib/evt.c	(revision 1420)
+++ lib/evt.c	(working copy)
@@ -110,10 +110,12 @@
  * ei_version:		version sent to the evtInitialize call.
  * ei_node_id:		our node id.
  * ei_node_name:	our node name.
- * ei_finalize:		instance in finalize flag
- * ei_dispatch_mutex:	mutex for dispatch fd
+ * ei_dispatch_mutex:	mutex for dispatch fd.  This lock also ensures that
+ *                      only one thread is using ei_dispatch_data.
  * ei_response_mutex:	mutex for response fd
  * ei_channel_list:		list of associated channels (struct handle_list)
+ * ei_dispatch_data:	event buffer for evtDispatch
+ * ei_finalize:		instance in finalize flag
  * ei_data_available:	Indicates that there is a pending event message thou=
gh
  * 						there may not be a poll event.  This can happen
  * 						when we get a SA_AIS_ERR_TRY_AGAIN when asking for an
@@ -127,11 +129,12 @@
 	SaVersionT				ei_version;
 	SaClmNodeIdT			ei_node_id;
 	SaNameT					ei_node_name;
-	int						ei_finalize;
 	pthread_mutex_t			ei_dispatch_mutex;
 	pthread_mutex_t			ei_response_mutex;
 	struct list_head 		ei_channel_list;
-	int						ei_data_available;
+	struct res_overlay		ei_dispatch_data;
+	unsigned int			ei_finalize:1;
+	unsigned int			ei_data_available:1;
 };
 =

 =

@@ -605,7 +608,6 @@
 	int ignore_dispatch =3D 0;
 	int cont =3D 1; /* always continue do loop except when set to 0 */
 	int poll_fd;
-	struct res_overlay dispatch_data;
 	struct lib_event_data *evt =3D 0;
 	struct res_evt_event_data res;
 =

@@ -680,15 +682,15 @@
 			}
 =

 			if (ufds.revents & POLLIN) {
-				error =3D saRecvRetry (evti->ei_dispatch_fd, &dispatch_data.header,
+				error =3D saRecvRetry (evti->ei_dispatch_fd, &evti->ei_dispatch_data.h=
eader,
 					sizeof (mar_res_header_t));
 =

 				if (error !=3D SA_AIS_OK) {
 					goto dispatch_unlock;
 				}
-				if (dispatch_data.header.size > sizeof (mar_res_header_t)) {
-					error =3D saRecvRetry (evti->ei_dispatch_fd, &dispatch_data.data,
-						dispatch_data.header.size - sizeof (mar_res_header_t));
+				if (evti->ei_dispatch_data.header.size > sizeof (mar_res_header_t)) {
+					error =3D saRecvRetry (evti->ei_dispatch_fd, &evti->ei_dispatch_data.=
data,
+						evti->ei_dispatch_data.header.size - sizeof (mar_res_header_t));
 					if (error !=3D SA_AIS_OK) {
 						goto dispatch_unlock;
 					}
@@ -703,7 +705,7 @@
 			 * Fake up a header message and the switch statement will
 			 * take care of the rest.
 			 */
-			dispatch_data.header.id =3D MESSAGE_RES_EVT_AVAILABLE;
+			evti->ei_dispatch_data.header.id =3D MESSAGE_RES_EVT_AVAILABLE;
 		}
 =

 		/*
@@ -713,13 +715,11 @@
 		 * EvtFinalize has been called in another thread.
 		 */
 		memcpy(&callbacks, &evti->ei_callback, sizeof(evti->ei_callback));
-		pthread_mutex_unlock(&evti->ei_dispatch_mutex);
 =

-
 		/*
 		 * Dispatch incoming response
 		 */
-		switch (dispatch_data.header.id) {
+		switch (evti->ei_dispatch_data.header.id) {
 =

 		case MESSAGE_RES_EVT_AVAILABLE:
 			evti->ei_data_available =3D 0;
@@ -788,7 +788,7 @@
 		case MESSAGE_RES_EVT_CHAN_OPEN_CALLBACK:
 		{
 			struct res_evt_open_chan_async *resa =3D =

-				(struct res_evt_open_chan_async *)&dispatch_data;
+				(struct res_evt_open_chan_async *)&evti->ei_dispatch_data;
 			struct event_channel_instance *eci;
 =

 			/*
@@ -821,11 +821,13 @@
 			break;
 =

 		default:
-			DPRINT (("Dispatch: Bad message type 0x%x\n", dispatch_data.header.id));
+			DPRINT (("Dispatch: Bad message type 0x%x\n", evti->ei_dispatch_data.he=
ader.id));
 			error =3D SA_AIS_ERR_LIBRARY;	=

-			goto dispatch_put;
+			goto dispatch_unlock;
 		}
 =

+		pthread_mutex_unlock(&evti->ei_dispatch_mutex);
+
 		/*
 		 * If empty is zero it means the we got the =

 		 * message from the queue and we are responsible
@@ -860,7 +862,7 @@
 	goto dispatch_put;
 =

 dispatch_unlock:
- 			pthread_mutex_unlock(&evti->ei_dispatch_mutex);
+ 	pthread_mutex_unlock(&evti->ei_dispatch_mutex);
 dispatch_put:
 	saHandleInstancePut(&evt_instance_handle_db, evtHandle);
 	return error;


More information about the Openais mailing list