[Xenomai-core] Allow "break" statement in RTDM_EXECUTE_ATOMICALLY()

[Xenomai-core] Allow "break" statement in RTDM_EXECUTE_ATOMICALLY()

[Philippe Gerum]

Jan,

Would you consider allowing "break" for leaving an atomic code block as
illustrated below?

I understand this would create a potential issue with newer RTDM drivers
relying on this feature when backported to former RTDM implementations,
but only having if/else constructs to control the execution flow within
an atomic block may be painful sometimes and lead to uselessly hairy
code, especially for error handling.
Maybe adding another helper macro with the desired behavior, and
documenting it separately from RTDM_EXECUTE_ATOMICALLY() would mitigate
the portability issue?

Additionally, I would definitely shadow/hide the spl value from the code
block in a way or another, since using "s" as a socket identifier for
RTDM-based protocol drivers is not that unusual.

e.g.

diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
index 058a9f8..18b6001 100644
--- a/include/rtdm/rtdm_driver.h
+++ b/include/rtdm/rtdm_driver.h
@@ -595,14 +595,16 @@ int rtdm_select_bind(int fd, rtdm_selector_t *selector,
 	<LEAVE_ATOMIC_SECTION>			\
 }
 #else /* This is how it really works */
-#define RTDM_EXECUTE_ATOMICALLY(code_block)	\
-{						\
+#define RTDM_ATOMIC_BLOCK(code_block)		\
+do {						\
 	spl_t s;				\
-						\
 	xnlock_get_irqsave(&nklock, s);		\
-	code_block;				\
+	do {					\
+	  code_block;				\
+	} while(0);				\
 	xnlock_put_irqrestore(&nklock, s);	\
-}
+} while(0)
+#define RTDM_EXECUTE_ATOMICALLY(code_block)	RTDM_ATOMIC_BLOCK(code_block)
 #endif
 /** @} Global Lock across Scheduler Invocation */
 
-- 
Philippe.

Re: [Xenomai-core] Allow "break" statement in RTDM_EXECUTE_ATOMICALLY()

[Jan Kiszka]

Philippe Gerum wrote:
> Jan,
> 
> Would you consider allowing "break" for leaving an atomic code block as
> illustrated below?
> 
> I understand this would create a potential issue with newer RTDM drivers
> relying on this feature when backported to former RTDM implementations,
> but only having if/else constructs to control the execution flow within
> an atomic block may be painful sometimes and lead to uselessly hairy
> code, especially for error handling.
> Maybe adding another helper macro with the desired behavior, and
> documenting it separately from RTDM_EXECUTE_ATOMICALLY() would mitigate
> the portability issue?
> 
> Additionally, I would definitely shadow/hide the spl value from the code
> block in a way or another, since using "s" as a socket identifier for
> RTDM-based protocol drivers is not that unusual.

I agree that "s" requires better encapsulation, that a do-while is a
good approach for this, and I would even change its name to something
like __rtdm_s.

But I don't think we should officially support "break" here (even if it
happens to work after refactoring). The macro does not expose explicit
"{ }" to the reader of the code that makes use of it (in contrast to
LIST_FOR_EACH-like helpers e.g.). Better use "goto" here, which is quite
common for error cleanup, too.

> 
> e.g.
> 
> diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h
> index 058a9f8..18b6001 100644
> --- a/include/rtdm/rtdm_driver.h
> +++ b/include/rtdm/rtdm_driver.h
> @@ -595,14 +595,16 @@ int rtdm_select_bind(int fd, rtdm_selector_t *selector,
>  	<LEAVE_ATOMIC_SECTION>			\
>  }
>  #else /* This is how it really works */
> -#define RTDM_EXECUTE_ATOMICALLY(code_block)	\
> -{						\
> +#define RTDM_ATOMIC_BLOCK(code_block)		\
> +do {						\
>  	spl_t s;				\
> -						\
>  	xnlock_get_irqsave(&nklock, s);		\
> -	code_block;				\
> +	do {					\
> +	  code_block;				\
> +	} while(0);				\
>  	xnlock_put_irqrestore(&nklock, s);	\
> -}
> +} while(0)
> +#define RTDM_EXECUTE_ATOMICALLY(code_block)	RTDM_ATOMIC_BLOCK(code_block)
>  #endif
>  /** @} Global Lock across Scheduler Invocation */
>  

Jan

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