| 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: <737664cf-bd1b-4ea7-9203-1a8e6a3473b7@kernel.org>
Date: Sun, 9 Nov 2025 10:42:07 -0700
From: David Ahern <dsahern@...nel.org>
To: Petr Oros <poros@...hat.com>, netdev@...r.kernel.org
Cc: stephen@...workplumber.org, jiri@...nulli.us,
Ivan Vecera <ivecera@...hat.com>
Subject: Re: [PATCH iproute2-next v2] dpll: Add dpll command
On 11/7/25 10:31 AM, Petr Oros wrote:
> diff --git a/dpll/dpll.c b/dpll/dpll.c
> new file mode 100644
> index 00000000000000..995f90b66759fa
> --- /dev/null
> +++ b/dpll/dpll.c
> @@ -0,0 +1,2022 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * dpll.c DPLL tool
> + *
> + * Authors: Petr Oros <poros@...hat.com>
> + */
> +
> +#include <errno.h>
> +#include <getopt.h>
> +#include <inttypes.h>
> +#include <poll.h>
> +#include <signal.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <linux/dpll.h>
> +#include <linux/genetlink.h>
> +#include <libmnl/libmnl.h>
> +
> +#include "../devlink/mnlg.h"
Add a separate patch that moves mnlg.c to lib, and mnlg.h to include.
> +#include "mnl_utils.h"
> +#include "version.h"
> +#include "utils.h"
> +#include "json_print.h"
> +
> +#define pr_err(args...) fprintf(stderr, ##args)
> +#define pr_out(args...) fprintf(stdout, ##args)
> +
> +struct dpll {
> + struct mnlu_gen_socket nlg;
> + int argc;
> + char **argv;
> + bool json_output;
> +};
> +
> +static volatile sig_atomic_t monitor_running = 1;
> +
> +static void monitor_sig_handler(int signo __attribute__((unused)))
> +{
> + monitor_running = 0;
> +}
> +
> +static int str_to_bool(const char *s, bool *val)
> +{
> + if (!strcmp(s, "true") || !strcmp(s, "1") || !strcmp(s, "enable"))
> + *val = true;
> + else if (!strcmp(s, "false") || !strcmp(s, "0") ||
> + !strcmp(s, "disable"))
> + *val = false;
> + else
> + return -EINVAL;
> + return 0;
> +}
This essentially replicates parse_one_of(). Make it a function in
lib/utils.c and update it to use parse_one_of.
...
> +
> +static int str_to_dpll_pin_state(const char *s, __u32 *v)
> +{
> + if (!strcmp(s, "connected"))
> + *v = DPLL_PIN_STATE_CONNECTED;
> + else if (!strcmp(s, "disconnected"))
> + *v = DPLL_PIN_STATE_DISCONNECTED;
> + else if (!strcmp(s, "selectable"))
> + *v = DPLL_PIN_STATE_SELECTABLE;
> + else
> + return -EINVAL;
> + return 0;
> +}
dpll_pin_state_name is the inverse of this; create a table that is used
for both directions.
> +
> +static int str_to_dpll_pin_type(const char *s, __u32 *type)
> +{
> + if (!strcmp(s, "mux"))
> + *type = DPLL_PIN_TYPE_MUX;
> + else if (!strcmp(s, "ext"))
> + *type = DPLL_PIN_TYPE_EXT;
> + else if (!strcmp(s, "synce-eth-port"))
> + *type = DPLL_PIN_TYPE_SYNCE_ETH_PORT;
> + else if (!strcmp(s, "int-oscillator"))
> + *type = DPLL_PIN_TYPE_INT_OSCILLATOR;
> + else if (!strcmp(s, "gnss"))
> + *type = DPLL_PIN_TYPE_GNSS;
> + else
> + return -EINVAL;
> + return 0;
> +}
dpll_pin_type_name below is the inverse of this. Do the same here - 1
table used by both directions.
> +
> +static int dpll_parse_state(struct dpll *dpll, __u32 *state)
> +{
> + const char *str = dpll_argv(dpll);
> +
> + if (str_to_dpll_pin_state(str, state)) {
> + pr_err("invalid state: %s (use connected/disconnected/selectable)\n",
> + str);
> + return -EINVAL;
> + }
> + dpll_arg_inc(dpll);
> + return 0;
> +}
> +
> +static int dpll_parse_direction(struct dpll *dpll, __u32 *direction)
> +{
> + if (dpll_argv_match_inc(dpll, "input")) {
> + *direction = DPLL_PIN_DIRECTION_INPUT;
> + } else if (dpll_argv_match_inc(dpll, "output")) {
> + *direction = DPLL_PIN_DIRECTION_OUTPUT;
> + } else {
> + pr_err("invalid direction: %s (use input/output)\n",
> + dpll_argv(dpll));
> + return -EINVAL;
> + }
> + return 0;
> +}
again here.
> +
> +static int dpll_parse_pin_type(struct dpll *dpll, __u32 *type)
> +{
> + const char *str = dpll_argv(dpll);
> +
> + if (str_to_dpll_pin_type(str, type)) {
> + pr_err("invalid type: %s (use mux/ext/synce-eth-port/int-oscillator/gnss)\n",> + str);
> + return -EINVAL;
> + }
> + dpll_arg_inc(dpll);
> + return 0;
> +}
> +
> +static int dpll_parse_u32(struct dpll *dpll, const char *arg_name,
> + __u32 *val_ptr)
> +{
> + const char *__str = dpll_argv_next(dpll);
> +
> + if (!__str) {
> + pr_err("%s requires an argument\n", arg_name);
> + return -EINVAL;
> + }
> + if (get_u32(val_ptr, __str, 0)) {
> + pr_err("invalid %s: %s\n", arg_name, __str);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int dpll_parse_attr_u32(struct dpll *dpll, struct nlmsghdr *nlh,
> + const char *arg_name, int attr_id)
> +{
> + __u32 val;
> +
> + if (dpll_parse_u32(dpll, arg_name, &val))
> + return -EINVAL;
> + mnl_attr_put_u32(nlh, attr_id, val);
> + return 0;
> +}
> +
> +static int dpll_parse_attr_s32(struct dpll *dpll, struct nlmsghdr *nlh,
> + const char *arg_name, int attr_id)
> +{
> + const char *str = dpll_argv_next(dpll);
> + __s32 val;
> +
> + if (!str) {
> + pr_err("%s requires an argument\n", arg_name);
> + return -EINVAL;
> + }
> + if (get_s32(&val, str, 0)) {
> + pr_err("invalid %s: %s\n", arg_name, str);
> + return -EINVAL;
> + }
> + mnl_attr_put_u32(nlh, attr_id, val);
function is `_s32` but the put here is `_u32`.
> + return 0;
> +}
> +
...
> +/* 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) \
INT
> + do { \
> + if (tb[attr_id]) \
> + print_int(PRINT_ANY, name, format_str, \
> + mnl_attr_get_u32(tb[attr_id])); \
u32?
> + } 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)
one user of mnl_attr_get_sint and it expects s64
> +{
> + 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)
> +
...
> +static int cmd_device_show(struct dpll *dpll)
> +{
> + bool has_id = false;
> + __u32 id = 0;
> +
> + while (dpll_argc(dpll) > 0) {
> + if (dpll_argv_match(dpll, "id")) {
> + if (dpll_parse_u32(dpll, "id", &id))
> + return -EINVAL;
> + has_id = true;
> + } else {
> + pr_err("unknown option: %s\n", dpll_argv(dpll));
> + return -EINVAL;
> + }
> + }
> +
> + if (has_id)
> + return cmd_device_show_id(dpll, id);
> + else
else is not needed, just
if ()
return...
return ...
> + return cmd_device_show_dump(dpll);
> +}
> +
A few "legacy" comments? this is the first submissions for this command
to iproute2, so how is there a legacy expectation?
> + pr_out(" ");
> + if (freq_min == freq_max) {
> + print_lluint(PRINT_FP, NULL, "%" PRIu64 " Hz\n",
> + freq_min);
> + } else {
> + print_lluint(PRINT_FP, NULL, "%" PRIu64,
> + freq_min);
> + pr_out("-");
> + print_lluint(PRINT_FP, NULL, "%" PRIu64 " Hz\n",
> + freq_max);
> + }
> + }
> +
Powered by blists - more mailing lists