| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5f1edf29-f83b-4dd5-b163-05d5e146bd50@redhat.com>
Date: Mon, 10 Nov 2025 14:17:06 +0100
From: Petr Oros <poros@...hat.com>
To: Stephen Hemminger <stephen@...workplumber.org>
Cc: netdev@...r.kernel.org, dsahern@...nel.org, jiri@...nulli.us,
Ivan Vecera <ivecera@...hat.com>
Subject: Re: [PATCH iproute2-next v2] dpll: Add dpll command
On 11/9/25 19:57, Stephen Hemminger wrote:
> On Fri, 7 Nov 2025 18:31:16 +0100
> Petr Oros <poros@...hat.com> wrote:
>
>> +
>> +/* Macros for printing netlink attributes
>> + * These macros combine the common pattern of:
>> + *
>> + * if (tb[ATTR])
>> + * print_xxx(PRINT_ANY, "name", "format", mnl_attr_get_xxx(tb[ATTR]));
>> + *
>> + * Generic versions with custom format string (_FMT suffix)
>> + * Simple versions auto-generate format string: " name: %d\n"
>> + */
>> +
>> +#define DPLL_PR_INT_FMT(tb, attr_id, name, format_str) \
>> + do { \
>> + if (tb[attr_id]) \
>> + print_int(PRINT_ANY, name, format_str, \
>> + mnl_attr_get_u32(tb[attr_id])); \
>> + } while (0)
>> +
>> +#define DPLL_PR_UINT_FMT(tb, attr_id, name, format_str) \
>> + do { \
>> + if (tb[attr_id]) \
>> + print_uint(PRINT_ANY, name, format_str, \
>> + mnl_attr_get_u32(tb[attr_id])); \
>> + } while (0)
>> +
>> +#define DPLL_PR_U64_FMT(tb, attr_id, name, format_str) \
>> + do { \
>> + if (tb[attr_id]) \
>> + print_lluint(PRINT_ANY, name, format_str, \
>> + mnl_attr_get_u64(tb[attr_id])); \
>> + } while (0)
>> +
>> +#define DPLL_PR_STR_FMT(tb, attr_id, name, format_str) \
>> + do { \
>> + if (tb[attr_id]) \
>> + print_string(PRINT_ANY, name, format_str, \
>> + mnl_attr_get_str(tb[attr_id])); \
>> + } while (0)
>> +
>> +/* Simple versions with auto-generated format */
>> +#define DPLL_PR_INT(tb, attr_id, name) \
>> + DPLL_PR_INT_FMT(tb, attr_id, name, " " name ": %d\n")
>> +
>> +#define DPLL_PR_UINT(tb, attr_id, name) \
>> + DPLL_PR_UINT_FMT(tb, attr_id, name, " " name ": %u\n")
>> +
>> +#define DPLL_PR_U64(tb, attr_id, name) \
>> + DPLL_PR_U64_FMT(tb, attr_id, name, " " name ": %" PRIu64 "\n")
>> +
>> +/* Helper to read signed int (can be s32 or s64 depending on value) */
>> +static __s64 mnl_attr_get_sint(const struct nlattr *attr)
>> +{
>> + if (mnl_attr_get_payload_len(attr) == sizeof(__s32))
>> + return *(__s32 *)mnl_attr_get_payload(attr);
>> + else
>> + return *(__s64 *)mnl_attr_get_payload(attr);
>> +}
>> +
>> +#define DPLL_PR_SINT_FMT(tb, attr_id, name, format_str) \
>> + do { \
>> + if (tb[attr_id]) \
>> + print_s64(PRINT_ANY, name, format_str, \
>> + mnl_attr_get_sint(tb[attr_id])); \
>> + } while (0)
>> +
>> +#define DPLL_PR_SINT(tb, attr_id, name) \
>> + DPLL_PR_SINT_FMT(tb, attr_id, name, " " name ": %" PRId64 "\n")
>> +
>> +#define DPLL_PR_STR(tb, attr_id, name) \
>> + DPLL_PR_STR_FMT(tb, attr_id, name, " " name ": %s\n")
>> +
>> +/* Temperature macro - JSON prints raw millidegrees, human prints formatted */
>> +#define DPLL_PR_TEMP(tb, attr_id) \
>> + do { \
>> + if (tb[attr_id]) { \
>> + __s32 temp = mnl_attr_get_u32(tb[attr_id]); \
>> + if (is_json_context()) { \
>> + print_int(PRINT_JSON, "temp", NULL, temp); \
>> + } else { \
>> + div_t d = div(temp, 1000); \
>> + pr_out(" temp: %d.%03d C\n", d.quot, d.rem); \
>> + } \
>> + } \
>> + } while (0)
>> +
>> +/* Generic version with custom format */
>> +#define DPLL_PR_ENUM_STR_FMT(tb, attr_id, name, format_str, name_func) \
>> + do { \
>> + if (tb[attr_id]) \
>> + print_string( \
>> + PRINT_ANY, name, format_str, \
>> + name_func(mnl_attr_get_u32(tb[attr_id]))); \
>> + } while (0)
>> +
>> +/* Simple version with auto-generated format */
>> +#define DPLL_PR_ENUM_STR(tb, attr_id, name, name_func) \
>> + DPLL_PR_ENUM_STR_FMT(tb, attr_id, name, " " name ": %s\n", name_func)
>> +
>> +/* Multi-attr enum printer - handles multiple occurrences of same attribute */
>> +#define DPLL_PR_MULTI_ENUM_STR(nlh, attr_id, name, name_func) \
>> + do { \
>> + struct nlattr *__attr; \
>> + bool __first = true; \
>> + \
>> + if (!nlh) \
>> + break; \
>> + \
>> + mnl_attr_for_each(__attr, nlh, sizeof(struct genlmsghdr)) \
>> + { \
>> + if (mnl_attr_get_type(__attr) == (attr_id)) { \
>> + __u32 __val = mnl_attr_get_u32(__attr); \
>> + if (__first) { \
>> + if (is_json_context()) { \
>> + open_json_array(PRINT_JSON, \
>> + name); \
>> + } else { \
>> + pr_out(" " name ":"); \
>> + } \
>> + __first = false; \
>> + } \
>> + if (is_json_context()) { \
>> + print_string(PRINT_JSON, NULL, NULL, \
>> + name_func(__val)); \
>> + } else { \
>> + pr_out(" %s", name_func(__val)); \
>> + } \
>> + } \
>> + } \
>> + if (__first) \
>> + break; \
>> + if (is_json_context()) { \
>> + close_json_array(PRINT_JSON, NULL); \
>> + } else { \
>> + pr_out("\n"); \
>> + } \
>> + } while (0)
>> +
> Code with large macros is harder to read and harder to maintain.
> Why can this not be inline functions?
> Why do you need to reinvent yet another netlink parser.
>
> Code that does is_json_context() is more verbose than necessary.
I understand the concern about the large macro. I used it mainly for
consistency with the existing DPLL_PR_* helpers, and it did not feel
excessively large in that context.
What I do not fully understand is the comment “Why do you need to
reinvent yet another netlink parser?”. The macros are only meant to
simplify the repeated pattern:
if (tb[ATTR]) print_xxx(PRINT_ANY, "name", "format",
mnl_attr_get_xxx(tb[ATTR]));
Is the issue that you would prefer to drop these macros entirely and use
this pattern
directly everywhere, or did you mean something else?
>
>> + if (is_json_context()) { \
>> + print_string(PRINT_JSON, NULL, NULL, \
>> + name_func(__val)); \
>> + } else { \
>> + pr_out(" %s", name_func(__val)); \
>> + }
> Can be replaced by:
> print_string(PRINT_ANY, NULL, " %s", name_func(__val));
ok, i will fix it in v3
>
> Please use existing code patterns.
>
Powered by blists - more mailing lists