diff mbox series

eal: simplify RTE_PMD_DEBUG_TRACE

Message ID 20181214125055.1153c38c@xeon-e3 (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series eal: simplify RTE_PMD_DEBUG_TRACE |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Stephen Hemminger Dec. 14, 2018, 8:50 p.m. UTC 
  Use rte_log directly, eliminating no longer used rte_pmd_dev_trace
function. This removes variable length array which is problem on
Windows and other compilers not doing C99.

Also, drop unused RTE_PROC_PRIMARY macros.

Reported-by: Jeff Shaw <jeffrey.b.shaw@intel.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_eal/common/include/rte_dev.h | 43 ++-----------------------
 1 file changed, 3 insertions(+), 40 deletions(-)

Comments

Jeff Shaw Dec. 14, 2018, 9:20 p.m. UTC | #1 
On Fri, Dec 14, 2018 at 12:50:55PM -0800, Stephen Hemminger wrote:
> Use rte_log directly, eliminating no longer used rte_pmd_dev_trace
> function. This removes variable length array which is problem on
> Windows and other compilers not doing C99.
> 
> Also, drop unused RTE_PROC_PRIMARY macros.
> 
> Reported-by: Jeff Shaw <jeffrey.b.shaw@intel.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_eal/common/include/rte_dev.h | 43 ++-----------------------
>  1 file changed, 3 insertions(+), 40 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index a9724dc9181c..e496da440028 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -43,54 +43,17 @@ typedef void (*rte_dev_event_cb_fn)(const char *device_name,
>  					enum rte_dev_event_type event,
>  					void *cb_arg);
>  
> -__attribute__((format(printf, 2, 0)))
> -static inline void
> -rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
> -{
> -	va_list ap;
> -
> -	va_start(ap, fmt);
> -
> -	{
> -		char buffer[vsnprintf(NULL, 0, fmt, ap) + 1];
> -
> -		va_end(ap);
> -
> -		va_start(ap, fmt);
> -		vsnprintf(buffer, sizeof(buffer), fmt, ap);
> -		va_end(ap);
> -
> -		rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s",
> -			func_name, buffer);
> -	}
> -}
> -

Will this break applications that try to use this function? Because it is not
a documented function, is there no guarantee it will be present?

>  /*
>   * Enable RTE_PMD_DEBUG_TRACE() when at least one component relying on the
>   * RTE_*_RET() macros defined below is compiled in debug mode.
>   */
>  #if defined(RTE_LIBRTE_EVENTDEV_DEBUG)
> -#define RTE_PMD_DEBUG_TRACE(...) \
> -	rte_pmd_debug_trace(__func__, __VA_ARGS__)
> +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> +	rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s():" fmt, __func__, ## args)

Actually, MSVC does not support named variable arguments either. I think this
will work instead:

  #define RTE_PMD_DEBUG_TRACE(fmt, ...) \
          rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s():" fmt, __func__, __VA_ARGS__)

The previous behavior was "%s: ..." not "%s():". I'm not sure if you meant to
change how the messages are displayed. I don't care either way, but maybe
users of the function would prefer the same format.
Stephen Hemminger Dec. 14, 2018, 9:57 p.m. UTC | #2 
On Fri, 14 Dec 2018 13:20:07 -0800
Jeff Shaw <jeffrey.b.shaw@intel.com> wrote:

> On Fri, Dec 14, 2018 at 12:50:55PM -0800, Stephen Hemminger wrote:
> > Use rte_log directly, eliminating no longer used rte_pmd_dev_trace
> > function. This removes variable length array which is problem on
> > Windows and other compilers not doing C99.
> > 
> > Also, drop unused RTE_PROC_PRIMARY macros.
> > 
> > Reported-by: Jeff Shaw <jeffrey.b.shaw@intel.com>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  lib/librte_eal/common/include/rte_dev.h | 43 ++-----------------------
> >  1 file changed, 3 insertions(+), 40 deletions(-)
> > 
> > diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> > index a9724dc9181c..e496da440028 100644
> > --- a/lib/librte_eal/common/include/rte_dev.h
> > +++ b/lib/librte_eal/common/include/rte_dev.h
> > @@ -43,54 +43,17 @@ typedef void (*rte_dev_event_cb_fn)(const char *device_name,
> >  					enum rte_dev_event_type event,
> >  					void *cb_arg);
> >  
> > -__attribute__((format(printf, 2, 0)))
> > -static inline void
> > -rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
> > -{
> > -	va_list ap;
> > -
> > -	va_start(ap, fmt);
> > -
> > -	{
> > -		char buffer[vsnprintf(NULL, 0, fmt, ap) + 1];
> > -
> > -		va_end(ap);
> > -
> > -		va_start(ap, fmt);
> > -		vsnprintf(buffer, sizeof(buffer), fmt, ap);
> > -		va_end(ap);
> > -
> > -		rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s",
> > -			func_name, buffer);
> > -	}
> > -}
> > -  
> 
> Will this break applications that try to use this function? Because it is not
> a documented function, is there no guarantee it will be present?

It shouldn't be visible as part of EAL. Any code that was built that had
old MACRO would still run (ABI compatible) because it was inline.
Ferruh Yigit Dec. 21, 2018, 4:17 p.m. UTC | #3 
On 12/14/2018 9:20 PM, Jeff Shaw wrote:
> On Fri, Dec 14, 2018 at 12:50:55PM -0800, Stephen Hemminger wrote:
>> Use rte_log directly, eliminating no longer used rte_pmd_dev_trace
>> function. This removes variable length array which is problem on
>> Windows and other compilers not doing C99.
>>
>> Also, drop unused RTE_PROC_PRIMARY macros.
>>
>> Reported-by: Jeff Shaw <jeffrey.b.shaw@intel.com>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> ---
>>  lib/librte_eal/common/include/rte_dev.h | 43 ++-----------------------
>>  1 file changed, 3 insertions(+), 40 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
>> index a9724dc9181c..e496da440028 100644
>> --- a/lib/librte_eal/common/include/rte_dev.h
>> +++ b/lib/librte_eal/common/include/rte_dev.h
>> @@ -43,54 +43,17 @@ typedef void (*rte_dev_event_cb_fn)(const char *device_name,
>>  					enum rte_dev_event_type event,
>>  					void *cb_arg);
>>  
>> -__attribute__((format(printf, 2, 0)))
>> -static inline void
>> -rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
>> -{
>> -	va_list ap;
>> -
>> -	va_start(ap, fmt);
>> -
>> -	{
>> -		char buffer[vsnprintf(NULL, 0, fmt, ap) + 1];
>> -
>> -		va_end(ap);
>> -
>> -		va_start(ap, fmt);
>> -		vsnprintf(buffer, sizeof(buffer), fmt, ap);
>> -		va_end(ap);
>> -
>> -		rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s",
>> -			func_name, buffer);
>> -	}
>> -}
>> -
> 
> Will this break applications that try to use this function? Because it is not
> a documented function, is there no guarantee it will be present?
> 
>>  /*
>>   * Enable RTE_PMD_DEBUG_TRACE() when at least one component relying on the
>>   * RTE_*_RET() macros defined below is compiled in debug mode.
>>   */
>>  #if defined(RTE_LIBRTE_EVENTDEV_DEBUG)
>> -#define RTE_PMD_DEBUG_TRACE(...) \
>> -	rte_pmd_debug_trace(__func__, __VA_ARGS__)
>> +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
>> +	rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s():" fmt, __func__, ## args)
> 
> Actually, MSVC does not support named variable arguments either. I think this
> will work instead:
> 
>   #define RTE_PMD_DEBUG_TRACE(fmt, ...) \
>           rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s():" fmt, __func__, __VA_ARGS__)
> 
> The previous behavior was "%s: ..." not "%s():". I'm not sure if you meant to
> change how the messages are displayed. I don't care either way, but maybe
> users of the function would prefer the same format.
> 

+1 to remove "rte_pmd_debug_trace()" option, but I guess a new version is
required, to switch to '__VA_ARGS__' and perhaps to keep the format same, %s vs
%s().

Who can send a new version? Jeff?
diff mbox series

Patch

diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index a9724dc9181c..e496da440028 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -43,54 +43,17 @@  typedef void (*rte_dev_event_cb_fn)(const char *device_name,
 					enum rte_dev_event_type event,
 					void *cb_arg);
 
-__attribute__((format(printf, 2, 0)))
-static inline void
-rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
-{
-	va_list ap;
-
-	va_start(ap, fmt);
-
-	{
-		char buffer[vsnprintf(NULL, 0, fmt, ap) + 1];
-
-		va_end(ap);
-
-		va_start(ap, fmt);
-		vsnprintf(buffer, sizeof(buffer), fmt, ap);
-		va_end(ap);
-
-		rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s",
-			func_name, buffer);
-	}
-}
-
 /*
  * Enable RTE_PMD_DEBUG_TRACE() when at least one component relying on the
  * RTE_*_RET() macros defined below is compiled in debug mode.
  */
 #if defined(RTE_LIBRTE_EVENTDEV_DEBUG)
-#define RTE_PMD_DEBUG_TRACE(...) \
-	rte_pmd_debug_trace(__func__, __VA_ARGS__)
+#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
+	rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s():" fmt, __func__, ## args)
 #else
-#define RTE_PMD_DEBUG_TRACE(...) (void)0
+#define RTE_PMD_DEBUG_TRACE(...) do { } while(0)
 #endif
 
-/* Macros for checking for restricting functions to primary instance only */
-#define RTE_PROC_PRIMARY_OR_ERR_RET(retval) do { \
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY) { \
-		RTE_PMD_DEBUG_TRACE("Cannot run in secondary processes\n"); \
-		return retval; \
-	} \
-} while (0)
-
-#define RTE_PROC_PRIMARY_OR_RET() do { \
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY) { \
-		RTE_PMD_DEBUG_TRACE("Cannot run in secondary processes\n"); \
-		return; \
-	} \
-} while (0)
-
 /* Macros to check for invalid function pointers */
 #define RTE_FUNC_PTR_OR_ERR_RET(func, retval) do { \
 	if ((func) == NULL) { \