[Xenomai-core] [PATCH 2/2] Provide owner name via rt_mutex_inquire

[Xenomai-core] [PATCH 2/2] Provide owner name via rt_mutex_inquire

[Jan Kiszka]

This can be helpful for debugging the (futile) release attempts of
mutexes by tasks that do not own them.

Returning the RT_TASK reference may appear more consistent on first
sight, but it cannot be guaranteed that the owner is actually a native
task. Therefore this patch uses the symbolic name.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---
 include/native/mutex.h    |    3 +++
 ksrc/skins/native/mutex.c |    5 +++++
 2 files changed, 8 insertions(+)

Index: b/include/native/mutex.h
===================================================================
--- a/include/native/mutex.h
+++ b/include/native/mutex.h
@@ -38,6 +38,9 @@ typedef struct rt_mutex_info {
 
 	char name[XNOBJECT_NAME_LEN]; /**< Symbolic name. */
 
+	char owner[XNOBJECT_NAME_LEN]; /**< Symbolic name of the current owner,
+					    empty if unlocked. */
+
 } RT_MUTEX_INFO;
 
 typedef struct rt_mutex_placeholder {
Index: b/ksrc/skins/native/mutex.c
===================================================================
--- a/ksrc/skins/native/mutex.c
+++ b/ksrc/skins/native/mutex.c
@@ -597,6 +597,11 @@ int rt_mutex_inquire(RT_MUTEX *mutex, RT
 	strcpy(info->name, mutex->name);
 	info->lockcnt = mutex->lockcnt;
 	info->nwaiters = xnsynch_nsleepers(&mutex->synch_base);
+	if (mutex->lockcnt)
+		strcpy(info->owner,
+		       xnthread_name(xnsynch_owner(&mutex->synch_base)));
+	else
+		info->owner[0] = 0;
 
       unlock_and_exit:
 

Re: [Xenomai-core] [PATCH 2/2] Provide owner name via rt_mutex_inquire

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> This can be helpful for debugging the (futile) release attempts of
> mutexes by tasks that do not own them.
> 
> Returning the RT_TASK reference may appear more consistent on first
> sight, but it cannot be guaranteed that the owner is actually a native
> task. Therefore this patch uses the symbolic name.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> ---
>  include/native/mutex.h    |    3 +++
>  ksrc/skins/native/mutex.c |    5 +++++
>  2 files changed, 8 insertions(+)
> 
> Index: b/include/native/mutex.h
> ===================================================================
> --- a/include/native/mutex.h
> +++ b/include/native/mutex.h
> @@ -38,6 +38,9 @@ typedef struct rt_mutex_info {
>  
>  	char name[XNOBJECT_NAME_LEN]; /**< Symbolic name. */
>  
> +	char owner[XNOBJECT_NAME_LEN]; /**< Symbolic name of the current owner,
> +					    empty if unlocked. */
> +
>  } RT_MUTEX_INFO;
>  
>  typedef struct rt_mutex_placeholder {
> Index: b/ksrc/skins/native/mutex.c
> ===================================================================
> --- a/ksrc/skins/native/mutex.c
> +++ b/ksrc/skins/native/mutex.c
> @@ -597,6 +597,11 @@ int rt_mutex_inquire(RT_MUTEX *mutex, RT
>  	strcpy(info->name, mutex->name);
>  	info->lockcnt = mutex->lockcnt;
>  	info->nwaiters = xnsynch_nsleepers(&mutex->synch_base);
> +	if (mutex->lockcnt)
> +		strcpy(info->owner,
> +		       xnthread_name(xnsynch_owner(&mutex->synch_base)));

xnthread_name is not necessarily null terminated, so I would suggest to use:

	snprintf(info->owner, sizeof(info->owner), "%s",
		 xnthread_name(xnsynch_owner(&mutex->synch_base)));

(and not strncpy, strncpy is crippled)

Another possibility would be to use snprintf instead of strncpy in 
xnobject_copy_name, and use xnobject_copy_name here.

-- 
                                                  Gilles.

Re: [Xenomai-core] [PATCH 2/2] Provide owner name via rt_mutex_inquire

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> This can be helpful for debugging the (futile) release attempts of
>> mutexes by tasks that do not own them.
>>
>> Returning the RT_TASK reference may appear more consistent on first
>> sight, but it cannot be guaranteed that the owner is actually a native
>> task. Therefore this patch uses the symbolic name.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>> ---
>>  include/native/mutex.h    |    3 +++
>>  ksrc/skins/native/mutex.c |    5 +++++
>>  2 files changed, 8 insertions(+)
>>
>> Index: b/include/native/mutex.h
>> ===================================================================
>> --- a/include/native/mutex.h
>> +++ b/include/native/mutex.h
>> @@ -38,6 +38,9 @@ typedef struct rt_mutex_info {
>>  
>>      char name[XNOBJECT_NAME_LEN]; /**< Symbolic name. */
>>  
>> +    char owner[XNOBJECT_NAME_LEN]; /**< Symbolic name of the current
>> owner,
>> +                        empty if unlocked. */
>> +
>>  } RT_MUTEX_INFO;
>>  
>>  typedef struct rt_mutex_placeholder {
>> Index: b/ksrc/skins/native/mutex.c
>> ===================================================================
>> --- a/ksrc/skins/native/mutex.c
>> +++ b/ksrc/skins/native/mutex.c
>> @@ -597,6 +597,11 @@ int rt_mutex_inquire(RT_MUTEX *mutex, RT
>>      strcpy(info->name, mutex->name);
>>      info->lockcnt = mutex->lockcnt;
>>      info->nwaiters = xnsynch_nsleepers(&mutex->synch_base);
>> +    if (mutex->lockcnt)
>> +        strcpy(info->owner,
>> +               xnthread_name(xnsynch_owner(&mutex->synch_base)));
> 
> xnthread_name is not necessarily null terminated, so I would suggest to

In fact, that is lethal for other users as well (trace points and proc
dumps come to my mind) and should be fixed ASAP.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH 2/2] Provide owner name via rt_mutex_inquire

[Gilles Chanteperdrix]

[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> This can be helpful for debugging the (futile) release attempts of
>>> mutexes by tasks that do not own them.
>>>
>>> Returning the RT_TASK reference may appear more consistent on first
>>> sight, but it cannot be guaranteed that the owner is actually a native
>>> task. Therefore this patch uses the symbolic name.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>> ---
>>>  include/native/mutex.h    |    3 +++
>>>  ksrc/skins/native/mutex.c |    5 +++++
>>>  2 files changed, 8 insertions(+)
>>>
>>> Index: b/include/native/mutex.h
>>> ===================================================================
>>> --- a/include/native/mutex.h
>>> +++ b/include/native/mutex.h
>>> @@ -38,6 +38,9 @@ typedef struct rt_mutex_info {
>>>  
>>>      char name[XNOBJECT_NAME_LEN]; /**< Symbolic name. */
>>>  
>>> +    char owner[XNOBJECT_NAME_LEN]; /**< Symbolic name of the current
>>> owner,
>>> +                        empty if unlocked. */
>>> +
>>>  } RT_MUTEX_INFO;
>>>  
>>>  typedef struct rt_mutex_placeholder {
>>> Index: b/ksrc/skins/native/mutex.c
>>> ===================================================================
>>> --- a/ksrc/skins/native/mutex.c
>>> +++ b/ksrc/skins/native/mutex.c
>>> @@ -597,6 +597,11 @@ int rt_mutex_inquire(RT_MUTEX *mutex, RT
>>>      strcpy(info->name, mutex->name);
>>>      info->lockcnt = mutex->lockcnt;
>>>      info->nwaiters = xnsynch_nsleepers(&mutex->synch_base);
>>> +    if (mutex->lockcnt)
>>> +        strcpy(info->owner,
>>> +               xnthread_name(xnsynch_owner(&mutex->synch_base)));
>> xnthread_name is not necessarily null terminated, so I would suggest to
> 
> In fact, that is lethal for other users as well (trace points and proc
> dumps come to my mind) and should be fixed ASAP.

I propose the following patch then. It should solve the issue at the source.

-- 
                                                  Gilles.

[-- Attachment #2: xeno-xnobject_copy_name-use-snprintf.diff --]
[-- Type: text/plain, Size: 472 bytes --]

Index: include/nucleus/types.h
===================================================================
--- include/nucleus/types.h	(r������vision 4004)
+++ include/nucleus/types.h	(copie de travail)
@@ -97,7 +97,7 @@ typedef atomic_flags_t xnflags_t;
 static inline void xnobject_copy_name(char *dst, const char *src)
 {
     if (src)
-        strncpy(dst, src, XNOBJECT_NAME_LEN);
+	snprintf(dst, XNOBJECT_NAME_LEN, "%s", src);
     else
         *dst = '\0';
 }

Re: [Xenomai-core] [PATCH 2/2] Provide owner name via rt_mutex_inquire

[Philippe Gerum]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> This can be helpful for debugging the (futile) release attempts of
>>> mutexes by tasks that do not own them.
>>>
>>> Returning the RT_TASK reference may appear more consistent on first
>>> sight, but it cannot be guaranteed that the owner is actually a native
>>> task. Therefore this patch uses the symbolic name.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>> ---
>>>  include/native/mutex.h    |    3 +++
>>>  ksrc/skins/native/mutex.c |    5 +++++
>>>  2 files changed, 8 insertions(+)
>>>
>>> Index: b/include/native/mutex.h
>>> ===================================================================
>>> --- a/include/native/mutex.h
>>> +++ b/include/native/mutex.h
>>> @@ -38,6 +38,9 @@ typedef struct rt_mutex_info {
>>>  
>>>      char name[XNOBJECT_NAME_LEN]; /**< Symbolic name. */
>>>  
>>> +    char owner[XNOBJECT_NAME_LEN]; /**< Symbolic name of the current
>>> owner,
>>> +                        empty if unlocked. */
>>> +
>>>  } RT_MUTEX_INFO;
>>>  
>>>  typedef struct rt_mutex_placeholder {
>>> Index: b/ksrc/skins/native/mutex.c
>>> ===================================================================
>>> --- a/ksrc/skins/native/mutex.c
>>> +++ b/ksrc/skins/native/mutex.c
>>> @@ -597,6 +597,11 @@ int rt_mutex_inquire(RT_MUTEX *mutex, RT
>>>      strcpy(info->name, mutex->name);
>>>      info->lockcnt = mutex->lockcnt;
>>>      info->nwaiters = xnsynch_nsleepers(&mutex->synch_base);
>>> +    if (mutex->lockcnt)
>>> +        strcpy(info->owner,
>>> +               xnthread_name(xnsynch_owner(&mutex->synch_base)));
>> xnthread_name is not necessarily null terminated, so I would suggest to
> 
> In fact, that is lethal for other users as well (trace points and proc
> dumps come to my mind) and should be fixed ASAP.
> 

xnthread_name() shall be considered as null terminated, the payload area is
actually XNOBJECT_NAME_LEN - 1. Users who do not assume this are the ones that
need fixing (like xnobject_copy_name).

-- 
Philippe.

Re: [Xenomai-core] [PATCH 2/2] Provide owner name via rt_mutex_inquire

[Gilles Chanteperdrix]

Gilles Chanteperdrix wrote:
> Another possibility would be to use snprintf instead of strncpy in 
> xnobject_copy_name, and use xnobject_copy_name here.

Yet another possibility would be to use strlcpy, available in 
kernel-space. But it is funny, glibc people does not seem to like 
strlcpy for reasons I do not quite understand...

http://sources.redhat.com/ml/libc-alpha/2000-08/msg00053.html

-- 
                                                  Gilles.

Re: [Xenomai-core] [PATCH 2/2] Provide owner name via rt_mutex_inquire

[Philippe Gerum]

Jan Kiszka wrote:
> This can be helpful for debugging the (futile) release attempts of
> mutexes by tasks that do not own them.
> 
> Returning the RT_TASK reference may appear more consistent on first
> sight, but it cannot be guaranteed that the owner is actually a native
> task. Therefore this patch uses the symbolic name.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 

Applied to the trunk/, thanks.

PS: no 2.4.x backport due to the ABI breakage this involves.

---
>  include/native/mutex.h    |    3 +++
>  ksrc/skins/native/mutex.c |    5 +++++
>  2 files changed, 8 insertions(+)
> 
> Index: b/include/native/mutex.h
> ===================================================================
> --- a/include/native/mutex.h
> +++ b/include/native/mutex.h
> @@ -38,6 +38,9 @@ typedef struct rt_mutex_info {
>  
>  	char name[XNOBJECT_NAME_LEN]; /**< Symbolic name. */
>  
> +	char owner[XNOBJECT_NAME_LEN]; /**< Symbolic name of the current owner,
> +					    empty if unlocked. */
> +
>  } RT_MUTEX_INFO;
>  
>  typedef struct rt_mutex_placeholder {
> Index: b/ksrc/skins/native/mutex.c
> ===================================================================
> --- a/ksrc/skins/native/mutex.c
> +++ b/ksrc/skins/native/mutex.c
> @@ -597,6 +597,11 @@ int rt_mutex_inquire(RT_MUTEX *mutex, RT
>  	strcpy(info->name, mutex->name);
>  	info->lockcnt = mutex->lockcnt;
>  	info->nwaiters = xnsynch_nsleepers(&mutex->synch_base);
> +	if (mutex->lockcnt)
> +		strcpy(info->owner,
> +		       xnthread_name(xnsynch_owner(&mutex->synch_base)));
> +	else
> +		info->owner[0] = 0;
>  
>        unlock_and_exit:
>  
> 
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core
> 


-- 
Philippe.