[v2,08/12] log: hide internal log structure
Checks
Commit Message
No need to expose rte_logs, hide it and remove it from the current ABI.
Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
---
Changelog since v1:
- updated release notes,
---
doc/guides/rel_notes/release_19_11.rst | 2 ++
lib/librte_eal/common/eal_common_log.c | 23 ++++++++++++++++-------
lib/librte_eal/common/include/rte_log.h | 20 +++-----------------
lib/librte_eal/rte_eal_version.map | 1 -
4 files changed, 21 insertions(+), 25 deletions(-)
Comments
23/10/2019 20:54, David Marchand:
> No need to expose rte_logs, hide it and remove it from the current ABI.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
[...]
> --- a/lib/librte_eal/common/include/rte_log.h
> +++ b/lib/librte_eal/common/include/rte_log.h
> -struct rte_log_dynamic_type;
> -
> -/** The rte_log structure. */
> -struct rte_logs {
> - uint32_t type; /**< Bitfield with enabled logs. */
> - uint32_t level; /**< Log level. */
> - FILE *file; /**< Output file set by rte_openlog_stream, or NULL. */
> - size_t dynamic_types_len;
> - struct rte_log_dynamic_type *dynamic_types;
> -};
I like this kind of change, but the FILE stream is available only through
the new experimental function. It is against the famous Mr Traynor rule:
we cannot deprecate or remove an old stable symbol if the replacement is experimental.
On 24/10/2019 17:30, Thomas Monjalon wrote:
> 23/10/2019 20:54, David Marchand:
>> No need to expose rte_logs, hide it and remove it from the current ABI.
>>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> [...]
>> --- a/lib/librte_eal/common/include/rte_log.h
>> +++ b/lib/librte_eal/common/include/rte_log.h
>> -struct rte_log_dynamic_type;
>> -
>> -/** The rte_log structure. */
>> -struct rte_logs {
>> - uint32_t type; /**< Bitfield with enabled logs. */
>> - uint32_t level; /**< Log level. */
>> - FILE *file; /**< Output file set by rte_openlog_stream, or NULL. */
>> - size_t dynamic_types_len;
>> - struct rte_log_dynamic_type *dynamic_types;
>> -};
>
> I like this kind of change, but the FILE stream is available only through
> the new experimental function. It is against the famous Mr Traynor rule:
> we cannot deprecate or remove an old stable symbol if the replacement is experimental.
>
>
For the change
Acked-by: Kevin Traynor <ktraynor@redhat.com>
++ for the rule (although s/we cannot/Thou shall not/ sounds more biblical)
As for accessor function being experimental, it is so simple I don't see
any issue with promoting it now. OTOH, if no one is planning to change
the struct anytime soon, it's probably fine to keep it public and
promote the fn. later.
@@ -210,6 +210,8 @@ ABI Changes
* eal: removed the ``rte_malloc_virt2phy`` function, replaced by
``rte_malloc_virt2iova`` since v17.11.
+* eal: made the ``rte_logs`` struct and global symbol private.
+
* pci: removed the following deprecated functions since dpdk:
- ``eal_parse_pci_BDF`` replaced by ``rte_pci_addr_parse``
@@ -17,13 +17,6 @@
#include "eal_private.h"
-/* global log structure */
-struct rte_logs rte_logs = {
- .type = ~0,
- .level = RTE_LOG_DEBUG,
- .file = NULL,
-};
-
struct rte_eal_opt_loglevel {
/** Next list entry */
TAILQ_ENTRY(rte_eal_opt_loglevel) next;
@@ -58,6 +51,22 @@ struct rte_log_dynamic_type {
uint32_t loglevel;
};
+/** The rte_log structure. */
+struct rte_logs {
+ uint32_t type; /**< Bitfield with enabled logs. */
+ uint32_t level; /**< Log level. */
+ FILE *file; /**< Output file set by rte_openlog_stream, or NULL. */
+ size_t dynamic_types_len;
+ struct rte_log_dynamic_type *dynamic_types;
+};
+
+/* global log structure */
+static struct rte_logs rte_logs = {
+ .type = ~0,
+ .level = RTE_LOG_DEBUG,
+ .file = NULL,
+};
+
/* per core log */
static RTE_DEFINE_PER_LCORE(struct log_cur_msg, log_cur_msg);
@@ -26,20 +26,6 @@ extern "C" {
#include <rte_config.h>
#include <rte_compat.h>
-struct rte_log_dynamic_type;
-
-/** The rte_log structure. */
-struct rte_logs {
- uint32_t type; /**< Bitfield with enabled logs. */
- uint32_t level; /**< Log level. */
- FILE *file; /**< Output file set by rte_openlog_stream, or NULL. */
- size_t dynamic_types_len;
- struct rte_log_dynamic_type *dynamic_types;
-};
-
-/** Global log information */
-extern struct rte_logs rte_logs;
-
/* SDK log type */
#define RTE_LOGTYPE_EAL 0 /**< Log related to eal. */
#define RTE_LOGTYPE_MALLOC 1 /**< Log related to malloc. */
@@ -260,7 +246,7 @@ void rte_log_dump(FILE *f);
* to rte_openlog_stream().
*
* The level argument determines if the log should be displayed or
- * not, depending on the global rte_logs variable.
+ * not, depending on the global log level and the per logtype level.
*
* The preferred alternative is the RTE_LOG() because it adds the
* level and type in the logged string.
@@ -291,8 +277,8 @@ int rte_log(uint32_t level, uint32_t logtype, const char *format, ...)
* to rte_openlog_stream().
*
* The level argument determines if the log should be displayed or
- * not, depending on the global rte_logs variable. A trailing
- * newline may be added if needed.
+ * not, depending on the global log level and the per logtype level.
+ * A trailing newline may be added if needed.
*
* The preferred alternative is the RTE_LOG() because it adds the
* level and type in the logged string.
@@ -45,7 +45,6 @@ DPDK_2.0 {
rte_log;
rte_log_cur_msg_loglevel;
rte_log_cur_msg_logtype;
- rte_logs;
rte_malloc;
rte_malloc_dump_stats;
rte_malloc_get_socket_stats;