Re: [PATCH 3/5] libcobalt: improve condvar autoinit support

[Jan Kiszka <jan.kiszka@web.de>]

On 07.03.19 14:21, Norbert Lange via Xenomai wrote:
> contrary to some comments, condvars are automatically
> initialised on signal/wait.
> Correct the destroy method to not report an
> error on such condvars.
>
> Check whether the condition variable has the
> static initializer is now more strict.
>

Some checkpatch warning are reported here as well. Not explicitly listing them.

> Signed-off-by: Norbert Lange <norbert.lange@andritz.com>
> ---
>   lib/cobalt/cond.c | 93 ++++++++++++++++++++++++++---------------------
>   1 file changed, 51 insertions(+), 42 deletions(-)
>
> diff --git a/lib/cobalt/cond.c b/lib/cobalt/cond.c
> index 00a201855..92eb230ff 100644
> --- a/lib/cobalt/cond.c
> +++ b/lib/cobalt/cond.c
> @@ -142,6 +142,42 @@ COBALT_IMPL(int, pthread_cond_init, (pthread_cond_t *cond,
>   	return 0;
>   }
>
> +static int __attribute__((cold)) _cobalt_cond_autoinit_type(const pthread_cond_t *cond)
> +{
> +	static const pthread_cond_t cond_initializers[] = {
> +		PTHREAD_COND_INITIALIZER
> +	};
> +	static const int cond_types[] =
> +	{
> +		0
> +	};
> +
> +	unsigned i = sizeof(cond_types) / sizeof(cond_types[0]);
> +	while (i-- > 0)
> +	{
> +		if (memcmp(cond, &cond_initializers[i], sizeof(cond_initializers[0])) == 0)
> +			return cond_types[i];
> +	}

While follows the same style as for mutexes, it's a bit overkill here, given
that we only have one type.

> +	return -1;
> +}
> +
> +static int __attribute__((cold)) _cobalt_cond_doautoinit(union cobalt_cond_union *ucond)
> +{
> +	int type;
> +	type = _cobalt_cond_autoinit_type(&ucond->native_cond);
> +	if (type < 0)
> +		return EINVAL;
> +
> +	return __COBALT(pthread_cond_init(&ucond->native_cond, NULL));
> +}
> +
> +static inline int _cobalt_cond_autoinit(union cobalt_cond_union *ucond)
> +{
> +	if (unlikely(ucond->shadow_cond.magic != COBALT_COND_MAGIC))
> +		return _cobalt_cond_doautoinit(ucond);
> +	return 0;
> +}
> +
>   /**
>    * @fn int pthread_cond_destroy(pthread_cond_t *cond)
>    * @brief Destroy a condition variable
> @@ -170,6 +206,9 @@ COBALT_IMPL(int, pthread_cond_destroy, (pthread_cond_t *cond))
>   {
>   	struct cobalt_cond_shadow *_cond = &((union cobalt_cond_union *)cond)->shadow_cond;
>
> +	if (unlikely(_cond->magic != COBALT_COND_MAGIC))
> +		return (_cobalt_cond_autoinit_type(cond) < 0) ? EINVAL : 0;
> +
>   	return -XENOMAI_SYSCALL1( sc_cobalt_cond_destroy, _cond);
>   }
>
> @@ -193,12 +232,6 @@ static void __pthread_cond_cleanup(void *data)
>   	c->mutex->lockcnt = c->count;
>   }
>
> -static int __attribute__((cold)) cobalt_cond_autoinit(pthread_cond_t *cond)
> -{
> -	return __COBALT(pthread_cond_init(cond, NULL));
> -}
> -
> -
>   /**
>    * Wait on a condition variable.
>    *
> @@ -262,10 +295,10 @@ COBALT_IMPL(int, pthread_cond_wait, (pthread_cond_t *cond, pthread_mutex_t *mute
>   	if (_mx->magic != COBALT_MUTEX_MAGIC)
>   		return EINVAL;
>
> -	if (_cnd->magic != COBALT_COND_MAGIC)
> -		goto autoinit;
> +	err = _cobalt_cond_autoinit((union cobalt_cond_union *)cond);
> +	if (err)
> +		return err;
>
> -  cont:
>   	if (_mx->attr.type == PTHREAD_MUTEX_ERRORCHECK) {
>   		xnhandle_t cur = cobalt_get_current();
>
> @@ -297,12 +330,6 @@ COBALT_IMPL(int, pthread_cond_wait, (pthread_cond_t *cond, pthread_mutex_t *mute
>   	pthread_testcancel();
>
>   	return -err ?: -c.err;
> -
> -  autoinit:
> -	err = cobalt_cond_autoinit(cond);
> -	if (err)
> -		return err;
> -	goto cont;
>   }
>
>   /**
> @@ -357,10 +384,10 @@ COBALT_IMPL(int, pthread_cond_timedwait, (pthread_cond_t *cond,
>   	if (_mx->magic != COBALT_MUTEX_MAGIC)
>   		return EINVAL;
>
> -	if (_cnd->magic != COBALT_COND_MAGIC)
> -		goto autoinit;
> +	err = _cobalt_cond_autoinit((union cobalt_cond_union *)cond);
> +	if (err)
> +		return err;
>
> -  cont:
>   	if (_mx->attr.type == PTHREAD_MUTEX_ERRORCHECK) {
>   		xnhandle_t cur = cobalt_get_current();
>
> @@ -391,12 +418,6 @@ COBALT_IMPL(int, pthread_cond_timedwait, (pthread_cond_t *cond,
>   	pthread_testcancel();
>
>   	return -err ?: -c.err;
> -
> -  autoinit:
> -	err = cobalt_cond_autoinit(cond);
> -	if (err)
> -		return err;
> -	goto cont;
>   }
>
>   /**
> @@ -431,10 +452,10 @@ COBALT_IMPL(int, pthread_cond_signal, (pthread_cond_t *cond))
>   	__u32 flags;
>   	int err;
>
> -	if (_cnd->magic != COBALT_COND_MAGIC)
> -		goto autoinit;
> +	err = _cobalt_cond_autoinit((union cobalt_cond_union *)cond);
> +	if (err)
> +		return err;
>
> -  cont:
>   	mutex_state = get_mutex_state(_cnd);
>   	if (mutex_state == NULL)
>   		return 0;	/* Fast path, no waiter. */
> @@ -455,12 +476,6 @@ COBALT_IMPL(int, pthread_cond_signal, (pthread_cond_t *cond))
>   		cond_state->pending_signals = pending_signals + 1;
>
>   	return 0;
> -
> -  autoinit:
> -	err = cobalt_cond_autoinit(cond);
> -	if (err)
> -		return err;
> -	goto cont;
>   }
>
>   /**
> @@ -491,10 +506,10 @@ COBALT_IMPL(int, pthread_cond_broadcast, (pthread_cond_t *cond))
>   	__u32 flags;
>   	int err;
>
> -	if (_cnd->magic != COBALT_COND_MAGIC)
> -		goto autoinit;
> +	err = _cobalt_cond_autoinit((union cobalt_cond_union *)cond);
> +	if (err)
> +		return err;
>
> -  cont:
>   	mutex_state = get_mutex_state(_cnd);
>   	if (mutex_state == NULL)
>   		return 0;
> @@ -513,12 +528,6 @@ COBALT_IMPL(int, pthread_cond_broadcast, (pthread_cond_t *cond))
>   	cond_state->pending_signals = ~0U;
>
>   	return 0;
> -
> -  autoinit:
> -	err = cobalt_cond_autoinit(cond);
> -	if (err)
> -		return err;
> -	goto cont;
>   }
>
>   /**
>

Jan