Re: [PATCH 2/6] Fix possible exit on error without releasing mutex

[John Kacur <jkacur@redhat.com>]

On Tue, 14 Jul 2015, Sebastian Andrzej Siewior wrote:

> * John Kacur | 2015-07-10 14:25:27 [+0200]:
> 
> >diff --git a/src/pi_tests/pi_stress.c b/src/pi_tests/pi_stress.c
> >index aaa36c362445..1d1cc58fae54 100644
> >--- a/src/pi_tests/pi_stress.c
> >+++ b/src/pi_tests/pi_stress.c
> >@@ -727,17 +727,24 @@ void *low_priority(void *arg)
> > 		status = pthread_barrier_wait(&p->locked_barrier);
> > 		if (status && status != PTHREAD_BARRIER_SERIAL_THREAD) {
> > 			pi_error
> >-			    ("low_priority[%d]: pthread_barrier_wait(locked): %x\n",
> >-			     p->id, status);
> >+				("low_priority[%d]: pthread_barrier_wait(locked): %x\n",
> >+				 p->id, status);
> >+			/* release the mutex */
> >+			pi_debug("low_priority[%d]: unlocking mutex\n", p->id);
> >+			pthread_mutex_unlock(&p->mutex);
> > 			return NULL;
> > 		}
> >+
> > 		/* wait for priority boost */
> > 		pi_debug("low_priority[%d]: entering elevated wait\n", p->id);
> > 		status = pthread_barrier_wait(&p->elevate_barrier);
> > 		if (status && status != PTHREAD_BARRIER_SERIAL_THREAD) {
> > 			pi_error
> >-			    ("low_priority[%d]: pthread_barrier_wait(elevate): %x\n",
> >-			     p->id, status);
> >+				("low_priority[%d]: pthread_barrier_wait(elevate): %x\n",
> >+				 p->id, status);
> >+			/* release the mutex */
> >+			pi_debug("low_priority[%d]: unlocking mutex\n", p->id);
> >+			pthread_mutex_unlock(&p->mutex);
> > 			return NULL;
> > 		}
> > 
> 
> What about having an out: label which does all the clean up at one spot
> instead of doing copy/paste here and there?
> 

The thought did cross my mind, there are certainly some classic parts of 
the kernel in which that provides a clean solution, but I'm not sure it's 
really better here.

We're talking about two unusual spots that basically are
line 1: print debug message
line 2: unlock the mutex
line 3: return null

With two spots that's a total of six lines, and the flow of control is 
quite clear.

Your method we have an out with 
line 1: out_label:
line 2: print debug message
line 3: unlock the mutex
line 4: return null

And then in two spots we have to do the
spot 1: goto out_label
spot 2: goto out_label

So the total number of lines is also six, but the flow of control isn't 
quite as clear.

It would make more sense if it was just part of the normal sequence in 
cleaning-up, but it isn't because for the other clean-up tasks we've
already released the mutex. Also, in case you are thinking that is a 
little odd, don't forget that this program purposely does some slightly 
odd things in order to intentionally trigger priority inversions.

That all being said, if you see a cleaner way to do this, I'll certainly
take a patch, but for now, I'm just going to supress the complaints from
the coverage tools.

John