* [PATCH net-next v2 1/8] net: netconsole: cleanup formatting lints
2024-01-26 23:13 [PATCH net-next v2 0/8] netconsole: Add userdata append support Matthew Wood
@ 2024-01-26 23:13 ` Matthew Wood
2024-01-30 9:23 ` Breno Leitao
2024-01-26 23:13 ` [PATCH net-next v2 2/8] net: netconsole: move netconsole_target config_item to config_group Matthew Wood
` (6 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Matthew Wood @ 2024-01-26 23:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: leitao, netdev, linux-kernel
Address checkpatch lint suggestions in preparation for later changes
Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
drivers/net/netconsole.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 6e14ba5e06c8..93fc3b509706 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -49,7 +49,7 @@ static char config[MAX_PARAM_LENGTH];
module_param_string(netconsole, config, MAX_PARAM_LENGTH, 0);
MODULE_PARM_DESC(netconsole, " netconsole=[src-port]@[src-ip]/[dev],[tgt-port]@<tgt-ip>/[tgt-macaddr]");
-static bool oops_only = false;
+static bool oops_only;
module_param(oops_only, bool, 0600);
MODULE_PARM_DESC(oops_only, "Only log oops messages");
@@ -501,6 +501,7 @@ static ssize_t local_ip_store(struct config_item *item, const char *buf,
if (strnchr(buf, count, ':')) {
const char *end;
+
if (in6_pton(buf, count, nt->np.local_ip.in6.s6_addr, -1, &end) > 0) {
if (*end && *end != '\n') {
pr_err("invalid IPv6 address at: <%c>\n", *end);
@@ -510,9 +511,9 @@ static ssize_t local_ip_store(struct config_item *item, const char *buf,
} else
goto out_unlock;
} else {
- if (!nt->np.ipv6) {
+ if (!nt->np.ipv6)
nt->np.local_ip.ip = in_aton(buf);
- } else
+ else
goto out_unlock;
}
@@ -537,6 +538,7 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf,
if (strnchr(buf, count, ':')) {
const char *end;
+
if (in6_pton(buf, count, nt->np.remote_ip.in6.s6_addr, -1, &end) > 0) {
if (*end && *end != '\n') {
pr_err("invalid IPv6 address at: <%c>\n", *end);
@@ -546,9 +548,9 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf,
} else
goto out_unlock;
} else {
- if (!nt->np.ipv6) {
+ if (!nt->np.ipv6)
nt->np.remote_ip.ip = in_aton(buf);
- } else
+ else
goto out_unlock;
}
@@ -781,6 +783,7 @@ static int netconsole_netdev_event(struct notifier_block *this,
spin_unlock_irqrestore(&target_list_lock, flags);
if (stopped) {
const char *msg = "had an event";
+
switch (event) {
case NETDEV_UNREGISTER:
msg = "unregistered";
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH net-next v2 2/8] net: netconsole: move netconsole_target config_item to config_group
2024-01-26 23:13 [PATCH net-next v2 0/8] netconsole: Add userdata append support Matthew Wood
2024-01-26 23:13 ` [PATCH net-next v2 1/8] net: netconsole: cleanup formatting lints Matthew Wood
@ 2024-01-26 23:13 ` Matthew Wood
2024-01-30 9:22 ` Breno Leitao
2024-02-02 11:54 ` Simon Horman
2024-01-26 23:13 ` [PATCH net-next v2 3/8] net: netconsole: move newline trimming to function Matthew Wood
` (5 subsequent siblings)
7 siblings, 2 replies; 20+ messages in thread
From: Matthew Wood @ 2024-01-26 23:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: leitao, netdev, linux-kernel
In order to support a nested userdata config_group in later patches,
use a config_group for netconsole_target instead of a
config_item. It's a no-op functionality-wise, since
config_group maintains all features of a config_item via the cg_item
member.
Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
drivers/net/netconsole.c | 61 ++++++++++++++++++++++------------------
1 file changed, 33 insertions(+), 28 deletions(-)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 93fc3b509706..085350beca87 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -79,7 +79,7 @@ static struct console netconsole_ext;
/**
* struct netconsole_target - Represents a configured netconsole target.
* @list: Links this target into the target_list.
- * @item: Links us into the configfs subsystem hierarchy.
+ * @group: Links us into the configfs subsystem hierarchy.
* @enabled: On / off knob to enable / disable target.
* Visible from userspace (read-write).
* We maintain a strict 1:1 correspondence between this and
@@ -102,7 +102,7 @@ static struct console netconsole_ext;
struct netconsole_target {
struct list_head list;
#ifdef CONFIG_NETCONSOLE_DYNAMIC
- struct config_item item;
+ struct config_group group;
#endif
bool enabled;
bool extended;
@@ -134,14 +134,14 @@ static void __exit dynamic_netconsole_exit(void)
*/
static void netconsole_target_get(struct netconsole_target *nt)
{
- if (config_item_name(&nt->item))
- config_item_get(&nt->item);
+ if (config_item_name(&nt->group.cg_item))
+ config_group_get(&nt->group);
}
static void netconsole_target_put(struct netconsole_target *nt)
{
- if (config_item_name(&nt->item))
- config_item_put(&nt->item);
+ if (config_item_name(&nt->group.cg_item))
+ config_group_put(&nt->group);
}
#else /* !CONFIG_NETCONSOLE_DYNAMIC */
@@ -221,9 +221,13 @@ static struct netconsole_target *alloc_and_init(void)
static struct netconsole_target *to_target(struct config_item *item)
{
- return item ?
- container_of(item, struct netconsole_target, item) :
- NULL;
+ struct config_group *cfg_group;
+
+ cfg_group = to_config_group(item);
+ if (!cfg_group)
+ return NULL;
+ return container_of(to_config_group(item),
+ struct netconsole_target, group);
}
/*
@@ -370,7 +374,7 @@ static ssize_t release_store(struct config_item *item, const char *buf,
mutex_lock(&dynamic_netconsole_mutex);
if (nt->enabled) {
pr_err("target (%s) is enabled, disable to update parameters\n",
- config_item_name(&nt->item));
+ config_item_name(&nt->group.cg_item));
err = -EINVAL;
goto out_unlock;
}
@@ -398,7 +402,7 @@ static ssize_t extended_store(struct config_item *item, const char *buf,
mutex_lock(&dynamic_netconsole_mutex);
if (nt->enabled) {
pr_err("target (%s) is enabled, disable to update parameters\n",
- config_item_name(&nt->item));
+ config_item_name(&nt->group.cg_item));
err = -EINVAL;
goto out_unlock;
}
@@ -425,7 +429,7 @@ static ssize_t dev_name_store(struct config_item *item, const char *buf,
mutex_lock(&dynamic_netconsole_mutex);
if (nt->enabled) {
pr_err("target (%s) is enabled, disable to update parameters\n",
- config_item_name(&nt->item));
+ config_item_name(&nt->group.cg_item));
mutex_unlock(&dynamic_netconsole_mutex);
return -EINVAL;
}
@@ -450,7 +454,7 @@ static ssize_t local_port_store(struct config_item *item, const char *buf,
mutex_lock(&dynamic_netconsole_mutex);
if (nt->enabled) {
pr_err("target (%s) is enabled, disable to update parameters\n",
- config_item_name(&nt->item));
+ config_item_name(&nt->group.cg_item));
goto out_unlock;
}
@@ -473,7 +477,7 @@ static ssize_t remote_port_store(struct config_item *item,
mutex_lock(&dynamic_netconsole_mutex);
if (nt->enabled) {
pr_err("target (%s) is enabled, disable to update parameters\n",
- config_item_name(&nt->item));
+ config_item_name(&nt->group.cg_item));
goto out_unlock;
}
@@ -495,7 +499,7 @@ static ssize_t local_ip_store(struct config_item *item, const char *buf,
mutex_lock(&dynamic_netconsole_mutex);
if (nt->enabled) {
pr_err("target (%s) is enabled, disable to update parameters\n",
- config_item_name(&nt->item));
+ config_item_name(&nt->group.cg_item));
goto out_unlock;
}
@@ -532,7 +536,7 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf,
mutex_lock(&dynamic_netconsole_mutex);
if (nt->enabled) {
pr_err("target (%s) is enabled, disable to update parameters\n",
- config_item_name(&nt->item));
+ config_item_name(&nt->group.cg_item));
goto out_unlock;
}
@@ -570,7 +574,7 @@ static ssize_t remote_mac_store(struct config_item *item, const char *buf,
mutex_lock(&dynamic_netconsole_mutex);
if (nt->enabled) {
pr_err("target (%s) is enabled, disable to update parameters\n",
- config_item_name(&nt->item));
+ config_item_name(&nt->group.cg_item));
goto out_unlock;
}
@@ -638,7 +642,7 @@ static struct netconsole_target *find_cmdline_target(const char *name)
spin_lock_irqsave(&target_list_lock, flags);
list_for_each_entry(nt, &target_list, list) {
- if (!strcmp(nt->item.ci_name, name)) {
+ if (!strcmp(nt->group.cg_item.ci_name, name)) {
ret = nt;
break;
}
@@ -652,8 +656,8 @@ static struct netconsole_target *find_cmdline_target(const char *name)
* Group operations and type for netconsole_subsys.
*/
-static struct config_item *make_netconsole_target(struct config_group *group,
- const char *name)
+static struct config_group *make_netconsole_target(struct config_group *group,
+ const char *name)
{
struct netconsole_target *nt;
unsigned long flags;
@@ -665,8 +669,9 @@ static struct config_item *make_netconsole_target(struct config_group *group,
if (!strncmp(name, NETCONSOLE_PARAM_TARGET_PREFIX,
strlen(NETCONSOLE_PARAM_TARGET_PREFIX))) {
nt = find_cmdline_target(name);
- if (nt)
- return &nt->item;
+ if (nt) {
+ return &nt->group;
+ }
}
nt = alloc_and_init();
@@ -674,14 +679,14 @@ static struct config_item *make_netconsole_target(struct config_group *group,
return ERR_PTR(-ENOMEM);
/* Initialize the config_item member */
- config_item_init_type_name(&nt->item, name, &netconsole_target_type);
+ config_group_init_type_name(&nt->group, name, &netconsole_target_type);
/* Adding, but it is disabled */
spin_lock_irqsave(&target_list_lock, flags);
list_add(&nt->list, &target_list);
spin_unlock_irqrestore(&target_list_lock, flags);
- return &nt->item;
+ return &nt->group;
}
static void drop_netconsole_target(struct config_group *group,
@@ -701,11 +706,11 @@ static void drop_netconsole_target(struct config_group *group,
if (nt->enabled)
netpoll_cleanup(&nt->np);
- config_item_put(&nt->item);
+ config_item_put(&nt->group.cg_item);
}
static struct configfs_group_operations netconsole_subsys_group_ops = {
- .make_item = make_netconsole_target,
+ .make_group = make_netconsole_target,
.drop_item = drop_netconsole_target,
};
@@ -731,8 +736,8 @@ static void populate_configfs_item(struct netconsole_target *nt,
snprintf(target_name, sizeof(target_name), "%s%d",
NETCONSOLE_PARAM_TARGET_PREFIX, cmdline_count);
- config_item_init_type_name(&nt->item, target_name,
- &netconsole_target_type);
+ config_group_init_type_name(&nt->group, target_name,
+ &netconsole_target_type);
}
#endif /* CONFIG_NETCONSOLE_DYNAMIC */
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH net-next v2 2/8] net: netconsole: move netconsole_target config_item to config_group
2024-01-26 23:13 ` [PATCH net-next v2 2/8] net: netconsole: move netconsole_target config_item to config_group Matthew Wood
@ 2024-01-30 9:22 ` Breno Leitao
2024-02-02 11:54 ` Simon Horman
1 sibling, 0 replies; 20+ messages in thread
From: Breno Leitao @ 2024-01-30 9:22 UTC (permalink / raw)
To: Matthew Wood
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel
On Fri, Jan 26, 2024 at 03:13:37PM -0800, Matthew Wood wrote:
> In order to support a nested userdata config_group in later patches,
> use a config_group for netconsole_target instead of a
> config_item. It's a no-op functionality-wise, since
> config_group maintains all features of a config_item via the cg_item
> member.
>
> Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
Reviewed-by: Breno Leitao <leitao@debian.org>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH net-next v2 2/8] net: netconsole: move netconsole_target config_item to config_group
2024-01-26 23:13 ` [PATCH net-next v2 2/8] net: netconsole: move netconsole_target config_item to config_group Matthew Wood
2024-01-30 9:22 ` Breno Leitao
@ 2024-02-02 11:54 ` Simon Horman
1 sibling, 0 replies; 20+ messages in thread
From: Simon Horman @ 2024-02-02 11:54 UTC (permalink / raw)
To: Matthew Wood
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
leitao, netdev, linux-kernel
On Fri, Jan 26, 2024 at 03:13:37PM -0800, Matthew Wood wrote:
> In order to support a nested userdata config_group in later patches,
> use a config_group for netconsole_target instead of a
> config_item. It's a no-op functionality-wise, since
> config_group maintains all features of a config_item via the cg_item
> member.
>
> Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
> ---
> drivers/net/netconsole.c | 61 ++++++++++++++++++++++------------------
> 1 file changed, 33 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
...
> @@ -665,8 +669,9 @@ static struct config_item *make_netconsole_target(struct config_group *group,
> if (!strncmp(name, NETCONSOLE_PARAM_TARGET_PREFIX,
> strlen(NETCONSOLE_PARAM_TARGET_PREFIX))) {
> nt = find_cmdline_target(name);
> - if (nt)
> - return &nt->item;
> + if (nt) {
> + return &nt->group;
> + }
nit: no need for {} here.
> }
>
> nt = alloc_and_init();
...
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH net-next v2 3/8] net: netconsole: move newline trimming to function
2024-01-26 23:13 [PATCH net-next v2 0/8] netconsole: Add userdata append support Matthew Wood
2024-01-26 23:13 ` [PATCH net-next v2 1/8] net: netconsole: cleanup formatting lints Matthew Wood
2024-01-26 23:13 ` [PATCH net-next v2 2/8] net: netconsole: move netconsole_target config_item to config_group Matthew Wood
@ 2024-01-26 23:13 ` Matthew Wood
2024-01-30 9:16 ` Breno Leitao
2024-01-26 23:13 ` [PATCH net-next v2 4/8] net: netconsole: add docs for appending netconsole user data Matthew Wood
` (4 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Matthew Wood @ 2024-01-26 23:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: leitao, netdev, linux-kernel
Move newline trimming logic from `dev_name_store()` to a new function
(trim_newline()) for shared use in netconsole.c
Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
drivers/net/netconsole.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 085350beca87..b280d06bf152 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -230,6 +230,16 @@ static struct netconsole_target *to_target(struct config_item *item)
struct netconsole_target, group);
}
+/* Get rid of possible trailing newline, returning the new length */
+static void trim_newline(char *s, size_t maxlen)
+{
+ size_t len;
+
+ len = strnlen(s, maxlen);
+ if (s[len - 1] == '\n')
+ s[len - 1] = '\0';
+}
+
/*
* Attribute operations for netconsole_target.
*/
@@ -424,7 +434,6 @@ static ssize_t dev_name_store(struct config_item *item, const char *buf,
size_t count)
{
struct netconsole_target *nt = to_target(item);
- size_t len;
mutex_lock(&dynamic_netconsole_mutex);
if (nt->enabled) {
@@ -435,11 +444,7 @@ static ssize_t dev_name_store(struct config_item *item, const char *buf,
}
strscpy(nt->np.dev_name, buf, IFNAMSIZ);
-
- /* Get rid of possible trailing newline from echo(1) */
- len = strnlen(nt->np.dev_name, IFNAMSIZ);
- if (nt->np.dev_name[len - 1] == '\n')
- nt->np.dev_name[len - 1] = '\0';
+ trim_newline(nt->np.dev_name, IFNAMSIZ);
mutex_unlock(&dynamic_netconsole_mutex);
return strnlen(buf, count);
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH net-next v2 3/8] net: netconsole: move newline trimming to function
2024-01-26 23:13 ` [PATCH net-next v2 3/8] net: netconsole: move newline trimming to function Matthew Wood
@ 2024-01-30 9:16 ` Breno Leitao
2024-02-01 4:45 ` Packet Geek
0 siblings, 1 reply; 20+ messages in thread
From: Breno Leitao @ 2024-01-30 9:16 UTC (permalink / raw)
To: Matthew Wood
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel
On Fri, Jan 26, 2024 at 03:13:38PM -0800, Matthew Wood wrote:
> Move newline trimming logic from `dev_name_store()` to a new function
> (trim_newline()) for shared use in netconsole.c
>
> Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
> ---
> drivers/net/netconsole.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 085350beca87..b280d06bf152 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -230,6 +230,16 @@ static struct netconsole_target *to_target(struct config_item *item)
> struct netconsole_target, group);
> }
>
> +/* Get rid of possible trailing newline, returning the new length */
> +static void trim_newline(char *s, size_t maxlen)
> +{
> + size_t len;
> +
> + len = strnlen(s, maxlen);
> + if (s[len - 1] == '\n')
> + s[len - 1] = '\0';
> +}
I am thinking about this one. Should we replace the first `\n` in the
file by `\0` no matter where it is? This will probably make it easier to
implement the netconsd, where we know it will be impossible to have `\n`
in the userdata.
Maybe something as:
static inline void trim_newline(char *str)
{
char *pos = strchr(str, '\n');
if (pos)
*pos = '\0';
}
All in all, this is a good clean up, which make the code easier to read.
Thanks!
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH net-next v2 3/8] net: netconsole: move newline trimming to function
2024-01-30 9:16 ` Breno Leitao
@ 2024-02-01 4:45 ` Packet Geek
2024-02-01 5:31 ` Matthew Wood
0 siblings, 1 reply; 20+ messages in thread
From: Packet Geek @ 2024-02-01 4:45 UTC (permalink / raw)
To: Breno Leitao
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel
On Tue, Jan 30, 2024 at 1:16 AM Breno Leitao <leitao@debian.org> wrote:
>
> On Fri, Jan 26, 2024 at 03:13:38PM -0800, Matthew Wood wrote:
> > Move newline trimming logic from `dev_name_store()` to a new function
> > (trim_newline()) for shared use in netconsole.c
> >
> > Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
> > ---
> > drivers/net/netconsole.c | 17 +++++++++++------
> > 1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 085350beca87..b280d06bf152 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -230,6 +230,16 @@ static struct netconsole_target *to_target(struct config_item *item)
> > struct netconsole_target, group);
> > }
> >
> > +/* Get rid of possible trailing newline, returning the new length */
> > +static void trim_newline(char *s, size_t maxlen)
> > +{
> > + size_t len;
> > +
> > + len = strnlen(s, maxlen);
> > + if (s[len - 1] == '\n')
> > + s[len - 1] = '\0';
> > +}
>
> I am thinking about this one. Should we replace the first `\n` in the
> file by `\0` no matter where it is? This will probably make it easier to
> implement the netconsd, where we know it will be impossible to have `\n`
> in the userdata.
>
> Maybe something as:
>
> static inline void trim_newline(char *str)
> {
> char *pos = strchr(str, '\n');
>
> if (pos)
> *pos = '\0';
> }
>
>
> All in all, this is a good clean up, which make the code easier to read.
> Thanks!
I like this idea, I agree that only accepting userdata values upto the
first newline clears up the expectations for log output and parsing on
the receiving side. I would prefer that to the case where multiple
values (delimited by newlines) are somehow attempted with a single
key, seems like just using additional key/value pairs would be
cleaner.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH net-next v2 3/8] net: netconsole: move newline trimming to function
2024-02-01 4:45 ` Packet Geek
@ 2024-02-01 5:31 ` Matthew Wood
0 siblings, 0 replies; 20+ messages in thread
From: Matthew Wood @ 2024-02-01 5:31 UTC (permalink / raw)
To: Breno Leitao
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel
On Wed, Jan 31, 2024 at 8:45 PM Matthew Wood <thepacketgeek@gmail.com> wrote:
>
> On Tue, Jan 30, 2024 at 1:16 AM Breno Leitao <leitao@debian.org> wrote:
> >
> > On Fri, Jan 26, 2024 at 03:13:38PM -0800, Matthew Wood wrote:
> > > Move newline trimming logic from `dev_name_store()` to a new function
> > > (trim_newline()) for shared use in netconsole.c
> > >
> > > Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
> > > ---
> > > drivers/net/netconsole.c | 17 +++++++++++------
> > > 1 file changed, 11 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > > index 085350beca87..b280d06bf152 100644
> > > --- a/drivers/net/netconsole.c
> > > +++ b/drivers/net/netconsole.c
> > > @@ -230,6 +230,16 @@ static struct netconsole_target *to_target(struct config_item *item)
> > > struct netconsole_target, group);
> > > }
> > >
> > > +/* Get rid of possible trailing newline, returning the new length */
> > > +static void trim_newline(char *s, size_t maxlen)
> > > +{
> > > + size_t len;
> > > +
> > > + len = strnlen(s, maxlen);
> > > + if (s[len - 1] == '\n')
> > > + s[len - 1] = '\0';
> > > +}
> >
> > I am thinking about this one. Should we replace the first `\n` in the
> > file by `\0` no matter where it is? This will probably make it easier to
> > implement the netconsd, where we know it will be impossible to have `\n`
> > in the userdata.
> >
> > Maybe something as:
> >
> > static inline void trim_newline(char *str)
> > {
> > char *pos = strchr(str, '\n');
> >
> > if (pos)
> > *pos = '\0';
> > }
> >
> >
> > All in all, this is a good clean up, which make the code easier to read.
> > Thanks!
>
> I like this idea, I agree that only accepting userdata values upto the
> first newline clears up the expectations for log output and parsing on
> the receiving side. I would prefer that to the case where multiple
> values (delimited by newlines) are somehow attempted with a single
> key, seems like just using additional key/value pairs would be
> cleaner.
In practice truncating at the first newline makes no difference as
printk, echo, and other methods seem to buffer and write per-line. So
in this example, the stored value will be "val2" with or without the
suggested change:
$ printf "val1\nval2" > userdata/testing/value
# This results in two calls to userdatum_value_store, the first with
"val1\n" and the second with "val2". "val2" remains as the latest
write.
$ cat userdata/testing/value
val2
I will add a warning about this possibly unexpected behavior in the
docs for v3 for the patch
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH net-next v2 4/8] net: netconsole: add docs for appending netconsole user data
2024-01-26 23:13 [PATCH net-next v2 0/8] netconsole: Add userdata append support Matthew Wood
` (2 preceding siblings ...)
2024-01-26 23:13 ` [PATCH net-next v2 3/8] net: netconsole: move newline trimming to function Matthew Wood
@ 2024-01-26 23:13 ` Matthew Wood
2024-01-26 23:13 ` [PATCH net-next v2 5/8] net: netconsole: add a userdata config_group member to netconsole_target Matthew Wood
` (3 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Matthew Wood @ 2024-01-26 23:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jonathan Corbet
Cc: leitao, netdev, linux-doc, linux-kernel
Add a new User Data section to the netconsole docs to describe the
appending of user data capability (for netconsole dynamic configuration)
with usage and netconsole output examples.
Co-developed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
Documentation/networking/netconsole.rst | 68 +++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/Documentation/networking/netconsole.rst b/Documentation/networking/netconsole.rst
index 390730a74332..54f072f47921 100644
--- a/Documentation/networking/netconsole.rst
+++ b/Documentation/networking/netconsole.rst
@@ -15,6 +15,8 @@ Extended console support by Tejun Heo <tj@kernel.org>, May 1 2015
Release prepend support by Breno Leitao <leitao@debian.org>, Jul 7 2023
+Userdata append support by Matthew Wood <thepacketgeek@gmail.com>, Jan 22 2024
+
Please send bug reports to Matt Mackall <mpm@selenic.com>
Satyam Sharma <satyam.sharma@gmail.com>, and Cong Wang <xiyou.wangcong@gmail.com>
@@ -171,6 +173,72 @@ You can modify these targets in runtime by creating the following targets::
cat cmdline1/remote_ip
10.0.0.3
+Append User Data
+================
+
+Custom user data can be appended to the end of messages with netconsole
+dynamic configuration enabled. User data entries can be modified without
+changing the "enabled" attribute of a target.
+
+Directories (keys) under `userdata` are limited to 54 character length, and
+data in `userdata/<key>/value` are limited to 200 bytes::
+
+ cd /sys/kernel/config/netconsole && mkdir cmdline0
+ cd cmdline0
+ mkdir userdata/foo
+ echo bar > userdata/foo/value
+ mkdir userdata/qux
+ echo baz > userdata/qux/value
+
+Messages will now include this additional user data::
+
+ echo "This is a message" > /dev/kmsg
+
+Sends::
+
+ 12,607,22085407756,-;This is a message
+ foo=bar
+ qux=baz
+
+Preview the userdata that will be appended with::
+
+ cd /sys/kernel/config/netconsole/cmdline0/userdata
+ for f in `ls userdata`; do echo $f=$(cat userdata/$f/value); done
+
+If a `userdata` entry is created but no data is written to the `value` file,
+the entry will be omitted from netconsole messages::
+
+ cd /sys/kernel/config/netconsole && mkdir cmdline0
+ cd cmdline0
+ mkdir userdata/foo
+ echo bar > userdata/foo/value
+ mkdir userdata/qux
+
+The `qux` key is omitted since it has no value::
+
+ echo "This is a message" > /dev/kmsg
+ 12,607,22085407756,-;This is a message
+ foo=bar
+
+Delete `userdata` entries with `rmdir`::
+
+ rmdir /sys/kernel/config/netconsole/cmdline0/userdata/qux
+
+Beware of `userdata` entries with newlines since values are printed with
+newlines preserved. A userdatum value with a newline could cause the
+netconsole message receiver to interpret a value as a new userdata key::
+
+ cat userdata/foo/value
+ bar
+ bar2
+
+The `qux` key is omitted since it has no value::
+
+ echo "This is a message" > /dev/kmsg
+ 12,607,22085407756,-;This is a message
+ foo=bar
+ bar2
+
Extended console:
=================
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH net-next v2 5/8] net: netconsole: add a userdata config_group member to netconsole_target
2024-01-26 23:13 [PATCH net-next v2 0/8] netconsole: Add userdata append support Matthew Wood
` (3 preceding siblings ...)
2024-01-26 23:13 ` [PATCH net-next v2 4/8] net: netconsole: add docs for appending netconsole user data Matthew Wood
@ 2024-01-26 23:13 ` Matthew Wood
2024-02-02 11:51 ` Simon Horman
2024-01-26 23:13 ` [PATCH net-next v2 6/8] net: netconsole: cache userdata formatted string in netconsole_target Matthew Wood
` (2 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Matthew Wood @ 2024-01-26 23:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: leitao, netdev, linux-kernel
Create configfs machinery for netconsole userdata appending, which depends
on CONFIG_NETCONSOLE_DYNAMIC (for configfs interface). Add a userdata
config_group to netconsole_target for managing userdata entries as a tree
under the netconsole configfs subsystem. Directory names created under the
userdata directory become userdatum keys; the userdatum value is the
content of the value file.
Include the minimum-viable-changes for userdata configfs config_group.
init_target_config_group() ties in the complete configfs machinery to
avoid unused func/variable errors during build. Initializing the
netconsole_target->group is moved to init_target_config_group, which
will also init and add the userdata config_group.
Each userdatum entry has a limit of 256 bytes (54 for
the key/directory, 200 for the value, and 2 for '=' and '\n'
characters), which is enforced by the configfs functions for updating
the userdata config_group.
When a new netconsole_target is created, initialize the userdata
config_group and add it as a default group for netconsole_target
config_group, allowing the userdata configfs sub-tree to be presented
in the netconsole configfs tree under the userdata directory.
Co-developed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
drivers/net/netconsole.c | 143 +++++++++++++++++++++++++++++++++++++--
1 file changed, 139 insertions(+), 4 deletions(-)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index b280d06bf152..a5ac21136f02 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -43,6 +43,10 @@ MODULE_DESCRIPTION("Console driver for network interfaces");
MODULE_LICENSE("GPL");
#define MAX_PARAM_LENGTH 256
+#define MAX_USERDATA_NAME_LENGTH 54
+#define MAX_USERDATA_VALUE_LENGTH 200
+#define MAX_USERDATA_ENTRY_LENGTH 256
+#define MAX_USERDATA_ITEMS 16
#define MAX_PRINT_CHUNK 1000
static char config[MAX_PARAM_LENGTH];
@@ -80,6 +84,7 @@ static struct console netconsole_ext;
* struct netconsole_target - Represents a configured netconsole target.
* @list: Links this target into the target_list.
* @group: Links us into the configfs subsystem hierarchy.
+ * @userdata_group: Links to the userdata configfs hierarchy
* @enabled: On / off knob to enable / disable target.
* Visible from userspace (read-write).
* We maintain a strict 1:1 correspondence between this and
@@ -103,6 +108,7 @@ struct netconsole_target {
struct list_head list;
#ifdef CONFIG_NETCONSOLE_DYNAMIC
struct config_group group;
+ struct config_group userdata_group;
#endif
bool enabled;
bool extended;
@@ -215,6 +221,10 @@ static struct netconsole_target *alloc_and_init(void)
* | remote_ip
* | local_mac
* | remote_mac
+ * | userdata/
+ * | <key>/
+ * | value
+ * | ...
* |
* <target>/...
*/
@@ -596,6 +606,123 @@ static ssize_t remote_mac_store(struct config_item *item, const char *buf,
return -EINVAL;
}
+struct userdatum {
+ struct config_item item;
+ char value[MAX_USERDATA_VALUE_LENGTH];
+};
+
+static inline struct userdatum *to_userdatum(struct config_item *item)
+{
+ return container_of(item, struct userdatum, item);
+}
+
+struct userdata {
+ struct config_group group;
+};
+
+static inline struct userdata *to_userdata(struct config_item *item)
+{
+ return container_of(to_config_group(item), struct userdata, group);
+}
+
+static struct netconsole_target *userdata_to_target(struct userdata *ud)
+{
+ struct config_group *netconsole_group;
+
+ netconsole_group = to_config_group(ud->group.cg_item.ci_parent);
+ return to_target(&netconsole_group->cg_item);
+}
+
+static ssize_t userdatum_value_show(struct config_item *item, char *buf)
+{
+ return sysfs_emit(buf, "%s\n", &(to_userdatum(item)->value[0]));
+}
+
+static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
+ size_t count)
+{
+ struct userdatum *udm = to_userdatum(item);
+ int ret;
+
+ if (count > MAX_USERDATA_VALUE_LENGTH)
+ return -EMSGSIZE;
+
+ mutex_lock(&dynamic_netconsole_mutex);
+
+ ret = strscpy(udm->value, buf, sizeof(udm->value));
+ if (ret < 0)
+ goto out_unlock;
+ trim_newline(udm->value, sizeof(udm->value));
+
+ mutex_unlock(&dynamic_netconsole_mutex);
+ return count;
+out_unlock:
+ mutex_unlock(&dynamic_netconsole_mutex);
+ return ret;
+}
+
+CONFIGFS_ATTR(userdatum_, value);
+
+static struct configfs_attribute *userdatum_attrs[] = {
+ &userdatum_attr_value,
+ NULL,
+};
+
+static void userdatum_release(struct config_item *item)
+{
+ kfree(to_userdatum(item));
+}
+
+static struct configfs_item_operations userdatum_ops = {
+ .release = userdatum_release,
+};
+
+static const struct config_item_type userdatum_type = {
+ .ct_item_ops = &userdatum_ops,
+ .ct_attrs = userdatum_attrs,
+ .ct_owner = THIS_MODULE,
+};
+
+static struct config_item *userdatum_make_item(struct config_group *group,
+ const char *name)
+{
+ struct netconsole_target *nt;
+ struct userdatum *udm;
+ struct userdata *ud;
+ size_t child_count;
+
+ if (strlen(name) > MAX_USERDATA_NAME_LENGTH)
+ return ERR_PTR(-ENAMETOOLONG);
+
+ ud = to_userdata(&group->cg_item);
+ nt = userdata_to_target(ud);
+ child_count = list_count_nodes(&nt->userdata_group.cg_children);
+ if (child_count >= MAX_USERDATA_ITEMS)
+ return ERR_PTR(-ENOSPC);
+
+ udm = kzalloc(sizeof(*udm), GFP_KERNEL);
+ if (!udm)
+ return ERR_PTR(-ENOMEM);
+
+ config_item_init_type_name(&udm->item, name, &userdatum_type);
+ return &udm->item;
+}
+
+static struct configfs_attribute *userdata_attrs[] = {
+ NULL,
+};
+
+static struct configfs_group_operations userdata_ops = {
+ .make_item = userdatum_make_item,
+};
+
+static struct config_item_type userdata_type = {
+ .ct_item_ops = &userdatum_ops,
+ .ct_group_ops = &userdata_ops,
+ .ct_attrs = userdata_attrs,
+ .ct_owner = THIS_MODULE,
+};
+
CONFIGFS_ATTR(, enabled);
CONFIGFS_ATTR(, extended);
CONFIGFS_ATTR(, dev_name);
@@ -640,6 +767,14 @@ static const struct config_item_type netconsole_target_type = {
.ct_owner = THIS_MODULE,
};
+static void init_target_config_group(struct netconsole_target *nt, const char *name)
+{
+ config_group_init_type_name(&nt->group, name, &netconsole_target_type);
+ config_group_init_type_name(&nt->userdata_group, "userdata",
+ &userdata_type);
+ configfs_add_default_group(&nt->userdata_group, &nt->group);
+}
+
static struct netconsole_target *find_cmdline_target(const char *name)
{
struct netconsole_target *nt, *ret = NULL;
@@ -675,6 +810,7 @@ static struct config_group *make_netconsole_target(struct config_group *group,
strlen(NETCONSOLE_PARAM_TARGET_PREFIX))) {
nt = find_cmdline_target(name);
if (nt) {
+ init_target_config_group(nt, name);
return &nt->group;
}
}
@@ -683,8 +819,8 @@ static struct config_group *make_netconsole_target(struct config_group *group,
if (!nt)
return ERR_PTR(-ENOMEM);
- /* Initialize the config_item member */
- config_group_init_type_name(&nt->group, name, &netconsole_target_type);
+ /* Initialize the config_group member */
+ init_target_config_group(nt, name);
/* Adding, but it is disabled */
spin_lock_irqsave(&target_list_lock, flags);
@@ -741,8 +877,7 @@ static void populate_configfs_item(struct netconsole_target *nt,
snprintf(target_name, sizeof(target_name), "%s%d",
NETCONSOLE_PARAM_TARGET_PREFIX, cmdline_count);
- config_group_init_type_name(&nt->group, target_name,
- &netconsole_target_type);
+ init_target_config_group(nt, target_name);
}
#endif /* CONFIG_NETCONSOLE_DYNAMIC */
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH net-next v2 5/8] net: netconsole: add a userdata config_group member to netconsole_target
2024-01-26 23:13 ` [PATCH net-next v2 5/8] net: netconsole: add a userdata config_group member to netconsole_target Matthew Wood
@ 2024-02-02 11:51 ` Simon Horman
2024-02-02 16:05 ` Matthew Wood
0 siblings, 1 reply; 20+ messages in thread
From: Simon Horman @ 2024-02-02 11:51 UTC (permalink / raw)
To: Matthew Wood
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
leitao, netdev, linux-kernel
On Fri, Jan 26, 2024 at 03:13:40PM -0800, Matthew Wood wrote:
> Create configfs machinery for netconsole userdata appending, which depends
> on CONFIG_NETCONSOLE_DYNAMIC (for configfs interface). Add a userdata
> config_group to netconsole_target for managing userdata entries as a tree
> under the netconsole configfs subsystem. Directory names created under the
> userdata directory become userdatum keys; the userdatum value is the
> content of the value file.
>
> Include the minimum-viable-changes for userdata configfs config_group.
> init_target_config_group() ties in the complete configfs machinery to
> avoid unused func/variable errors during build. Initializing the
> netconsole_target->group is moved to init_target_config_group, which
> will also init and add the userdata config_group.
>
> Each userdatum entry has a limit of 256 bytes (54 for
> the key/directory, 200 for the value, and 2 for '=' and '\n'
> characters), which is enforced by the configfs functions for updating
> the userdata config_group.
>
> When a new netconsole_target is created, initialize the userdata
> config_group and add it as a default group for netconsole_target
> config_group, allowing the userdata configfs sub-tree to be presented
> in the netconsole configfs tree under the userdata directory.
>
> Co-developed-by: Breno Leitao <leitao@debian.org>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
Hi Matthew,
some minor feedback from my side, as it looks like there will be another
revision of this patchset anyway.
> ---
> drivers/net/netconsole.c | 143 +++++++++++++++++++++++++++++++++++++--
> 1 file changed, 139 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
...
> @@ -596,6 +606,123 @@ static ssize_t remote_mac_store(struct config_item *item, const char *buf,
> return -EINVAL;
> }
>
> +struct userdatum {
> + struct config_item item;
> + char value[MAX_USERDATA_VALUE_LENGTH];
> +};
> +
> +static inline struct userdatum *to_userdatum(struct config_item *item)
> +{
> + return container_of(item, struct userdatum, item);
> +}
Please don't use the inline keyword in C files,
unless there is a demonstrable reason to do so.
Rather, please let the compiler inline code as is sees fit.
...
> @@ -640,6 +767,14 @@ static const struct config_item_type netconsole_target_type = {
> .ct_owner = THIS_MODULE,
> };
>
> +static void init_target_config_group(struct netconsole_target *nt, const char *name)
nit: Networking still prefers code to be 80 columns wide or less.
...
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH net-next v2 5/8] net: netconsole: add a userdata config_group member to netconsole_target
2024-02-02 11:51 ` Simon Horman
@ 2024-02-02 16:05 ` Matthew Wood
2024-02-06 13:51 ` Simon Horman
0 siblings, 1 reply; 20+ messages in thread
From: Matthew Wood @ 2024-02-02 16:05 UTC (permalink / raw)
To: Simon Horman
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
leitao, netdev, linux-kernel
On Fri, Feb 2, 2024 at 3:52 AM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Jan 26, 2024 at 03:13:40PM -0800, Matthew Wood wrote:
> > Create configfs machinery for netconsole userdata appending, which depends
> > on CONFIG_NETCONSOLE_DYNAMIC (for configfs interface). Add a userdata
> > config_group to netconsole_target for managing userdata entries as a tree
> > under the netconsole configfs subsystem. Directory names created under the
> > userdata directory become userdatum keys; the userdatum value is the
> > content of the value file.
> >
> > Include the minimum-viable-changes for userdata configfs config_group.
> > init_target_config_group() ties in the complete configfs machinery to
> > avoid unused func/variable errors during build. Initializing the
> > netconsole_target->group is moved to init_target_config_group, which
> > will also init and add the userdata config_group.
> >
> > Each userdatum entry has a limit of 256 bytes (54 for
> > the key/directory, 200 for the value, and 2 for '=' and '\n'
> > characters), which is enforced by the configfs functions for updating
> > the userdata config_group.
> >
> > When a new netconsole_target is created, initialize the userdata
> > config_group and add it as a default group for netconsole_target
> > config_group, allowing the userdata configfs sub-tree to be presented
> > in the netconsole configfs tree under the userdata directory.
> >
> > Co-developed-by: Breno Leitao <leitao@debian.org>
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
>
> Hi Matthew,
>
> some minor feedback from my side, as it looks like there will be another
> revision of this patchset anyway.
>
> > ---
> > drivers/net/netconsole.c | 143 +++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 139 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
>
> ...
>
> > @@ -596,6 +606,123 @@ static ssize_t remote_mac_store(struct config_item *item, const char *buf,
> > return -EINVAL;
> > }
> >
> > +struct userdatum {
> > + struct config_item item;
> > + char value[MAX_USERDATA_VALUE_LENGTH];
> > +};
> > +
> > +static inline struct userdatum *to_userdatum(struct config_item *item)
> > +{
> > + return container_of(item, struct userdatum, item);
> > +}
>
> Please don't use the inline keyword in C files,
> unless there is a demonstrable reason to do so.
> Rather, please let the compiler inline code as is sees fit.
>
> ...
>
> > @@ -640,6 +767,14 @@ static const struct config_item_type netconsole_target_type = {
> > .ct_owner = THIS_MODULE,
> > };
> >
> > +static void init_target_config_group(struct netconsole_target *nt, const char *name)
>
> nit: Networking still prefers code to be 80 columns wide or less.
>
> ...
Hi Simon,
I appreciate the review, thank you for the feedback. I've addressed
the comments here and in the other patches too. I'll be posting a v3
soon with the changes.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH net-next v2 5/8] net: netconsole: add a userdata config_group member to netconsole_target
2024-02-02 16:05 ` Matthew Wood
@ 2024-02-06 13:51 ` Simon Horman
0 siblings, 0 replies; 20+ messages in thread
From: Simon Horman @ 2024-02-06 13:51 UTC (permalink / raw)
To: Matthew Wood
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
leitao, netdev, linux-kernel
On Fri, Feb 02, 2024 at 08:05:07AM -0800, Matthew Wood wrote:
...
> Hi Simon,
>
> I appreciate the review, thank you for the feedback. I've addressed
> the comments here and in the other patches too. I'll be posting a v3
> soon with the changes.
>
Thanks Matthew,
likewise, much appreciated.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH net-next v2 6/8] net: netconsole: cache userdata formatted string in netconsole_target
2024-01-26 23:13 [PATCH net-next v2 0/8] netconsole: Add userdata append support Matthew Wood
` (4 preceding siblings ...)
2024-01-26 23:13 ` [PATCH net-next v2 5/8] net: netconsole: add a userdata config_group member to netconsole_target Matthew Wood
@ 2024-01-26 23:13 ` Matthew Wood
2024-01-27 13:15 ` kernel test robot
2024-02-02 11:53 ` Simon Horman
2024-01-26 23:13 ` [PATCH net-next v2 7/8] net: netconsole: append userdata to netconsole messages Matthew Wood
2024-01-26 23:13 ` [PATCH net-next v2 8/8] net: netconsole: append userdata to fragmented " Matthew Wood
7 siblings, 2 replies; 20+ messages in thread
From: Matthew Wood @ 2024-01-26 23:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: leitao, netdev, linux-kernel
Store a formatted string for userdata that will be appended to netconsole
messages. The string has a capacity of 4KB, as calculated by the userdatum
entry length of 256 bytes and a max of 16 userdata entries.
Update the stored netconsole_target->userdata_complete string with the new
formatted userdata values when a userdatum is created, edited, or
removed. Each userdata entry contains a trailing newline, which will be
formatted as such in netconsole messages::
6.7.0-rc8-virtme,12,500,1646292204,-;test
release=foo
something=bar
6.7.0-rc8-virtme,12,500,1646292204,-;another test
release=foo
something=bar
Enforcement of MAX_USERDATA_ITEMS is done in userdatum_make_item;
update_userdata will not check for this case but will skip any userdata
children over the limit of MAX_USERDATA_ITEMs.
If a userdata entry/dir is created but no value is provided, that entry
will be skipped. This is in part because update_userdata() can't be
called in userdatum_make_item() since the item will not have been added
to the userdata config_group children yet. To preserve the experience of
adding an empty userdata that doesn't show up in the netconsole
messages, purposefully skip emtpy userdata items even when
update_userdata() can be called.
Co-developed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
drivers/net/netconsole.c | 63 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 63 insertions(+)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index a5ac21136f02..73feba0b3c93 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -85,6 +85,8 @@ static struct console netconsole_ext;
* @list: Links this target into the target_list.
* @group: Links us into the configfs subsystem hierarchy.
* @userdata_group: Links to the userdata configfs hierarchy
+ * @userdata_complete: Cached, formatted string of append
+ * @userdata_length: String length of userdata_complete
* @enabled: On / off knob to enable / disable target.
* Visible from userspace (read-write).
* We maintain a strict 1:1 correspondence between this and
@@ -109,6 +111,8 @@ struct netconsole_target {
#ifdef CONFIG_NETCONSOLE_DYNAMIC
struct config_group group;
struct config_group userdata_group;
+ char userdata_complete[MAX_USERDATA_ENTRY_LENGTH * MAX_USERDATA_ITEMS];
+ size_t userdata_length;
#endif
bool enabled;
bool extended;
@@ -638,10 +642,50 @@ static ssize_t userdatum_value_show(struct config_item *item, char *buf)
return sysfs_emit(buf, "%s\n", &(to_userdatum(item)->value[0]));
}
+static void update_userdata(struct netconsole_target *nt)
+{
+ int complete_idx = 0, child_count = 0;
+ struct list_head *entry;
+ struct userdata *ud;
+
+ /* Clear the current string in case the last userdatum was deleted */
+ nt->userdata_length = 0;
+ nt->userdata_complete[0] = 0;
+
+ ud = to_userdata(&nt->userdata_group.cg_item);
+ list_for_each(entry, &nt->userdata_group.cg_children) {
+ struct userdatum *udm_item;
+ struct config_item *item;
+
+ if (child_count >= MAX_USERDATA_ITEMS)
+ break;
+ child_count++;
+
+ item = container_of(entry, struct config_item, ci_entry);
+ udm_item = to_userdatum(item);
+
+ /* Skip userdata with no value set */
+ if (strnlen(udm_item->value, MAX_USERDATA_VALUE_LENGTH) == 0)
+ continue;
+
+ /* This doesn't overflow userdata_complete since it will write
+ * one entry length (1/MAX_USERDATA_ITEMS long), entry count is
+ * checked to not exceed MAX items with child_count above
+ */
+ complete_idx += scnprintf(&nt->userdata_complete[complete_idx],
+ MAX_USERDATA_ENTRY_LENGTH, "%s=%s\n",
+ item->ci_name, udm_item->value);
+ }
+ nt->userdata_length = strnlen(nt->userdata_complete,
+ sizeof(nt->userdata_complete));
+}
+
static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
size_t count)
{
struct userdatum *udm = to_userdatum(item);
+ struct netconsole_target *nt;
+ struct userdata *ud;
int ret;
if (count > MAX_USERDATA_VALUE_LENGTH)
@@ -654,6 +698,10 @@ static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
goto out_unlock;
trim_newline(udm->value, sizeof(udm->value));
+ ud = to_userdata(item->ci_parent);
+ nt = userdata_to_target(ud);
+ update_userdata(nt);
+
mutex_unlock(&dynamic_netconsole_mutex);
return count;
out_unlock:
@@ -708,12 +756,27 @@ static struct config_item *userdatum_make_item(struct config_group *group,
return &udm->item;
}
+static void userdatum_drop(struct config_group *group, struct config_item *item)
+{
+ struct netconsole_target *nt;
+ struct userdata *ud;
+
+ ud = to_userdata(&group->cg_item);
+ nt = userdata_to_target(ud);
+
+ mutex_lock(&dynamic_netconsole_mutex);
+ update_userdata(nt);
+ config_item_put(item);
+ mutex_unlock(&dynamic_netconsole_mutex);
+}
+
static struct configfs_attribute *userdata_attrs[] = {
NULL,
};
static struct configfs_group_operations userdata_ops = {
.make_item = userdatum_make_item,
+ .drop_item = userdatum_drop,
};
static struct config_item_type userdata_type = {
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH net-next v2 6/8] net: netconsole: cache userdata formatted string in netconsole_target
2024-01-26 23:13 ` [PATCH net-next v2 6/8] net: netconsole: cache userdata formatted string in netconsole_target Matthew Wood
@ 2024-01-27 13:15 ` kernel test robot
2024-02-02 11:53 ` Simon Horman
1 sibling, 0 replies; 20+ messages in thread
From: kernel test robot @ 2024-01-27 13:15 UTC (permalink / raw)
To: Matthew Wood, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: oe-kbuild-all, netdev, leitao, linux-kernel
Hi Matthew,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Matthew-Wood/net-netconsole-cleanup-formatting-lints/20240127-072017
base: net-next/main
patch link: https://lore.kernel.org/r/20240126231348.281600-7-thepacketgeek%40gmail.com
patch subject: [PATCH net-next v2 6/8] net: netconsole: cache userdata formatted string in netconsole_target
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20240127/202401272022.r5Z4OtUg-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240127/202401272022.r5Z4OtUg-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401272022.r5Z4OtUg-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/net/netconsole.c: In function 'update_userdata':
>> drivers/net/netconsole.c:649:26: warning: variable 'ud' set but not used [-Wunused-but-set-variable]
649 | struct userdata *ud;
| ^~
vim +/ud +649 drivers/net/netconsole.c
644
645 static void update_userdata(struct netconsole_target *nt)
646 {
647 int complete_idx = 0, child_count = 0;
648 struct list_head *entry;
> 649 struct userdata *ud;
650
651 /* Clear the current string in case the last userdatum was deleted */
652 nt->userdata_length = 0;
653 nt->userdata_complete[0] = 0;
654
655 ud = to_userdata(&nt->userdata_group.cg_item);
656 list_for_each(entry, &nt->userdata_group.cg_children) {
657 struct userdatum *udm_item;
658 struct config_item *item;
659
660 if (child_count >= MAX_USERDATA_ITEMS)
661 break;
662 child_count++;
663
664 item = container_of(entry, struct config_item, ci_entry);
665 udm_item = to_userdatum(item);
666
667 /* Skip userdata with no value set */
668 if (strnlen(udm_item->value, MAX_USERDATA_VALUE_LENGTH) == 0)
669 continue;
670
671 /* This doesn't overflow userdata_complete since it will write
672 * one entry length (1/MAX_USERDATA_ITEMS long), entry count is
673 * checked to not exceed MAX items with child_count above
674 */
675 complete_idx += scnprintf(&nt->userdata_complete[complete_idx],
676 MAX_USERDATA_ENTRY_LENGTH, "%s=%s\n",
677 item->ci_name, udm_item->value);
678 }
679 nt->userdata_length = strnlen(nt->userdata_complete,
680 sizeof(nt->userdata_complete));
681 }
682
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH net-next v2 6/8] net: netconsole: cache userdata formatted string in netconsole_target
2024-01-26 23:13 ` [PATCH net-next v2 6/8] net: netconsole: cache userdata formatted string in netconsole_target Matthew Wood
2024-01-27 13:15 ` kernel test robot
@ 2024-02-02 11:53 ` Simon Horman
1 sibling, 0 replies; 20+ messages in thread
From: Simon Horman @ 2024-02-02 11:53 UTC (permalink / raw)
To: Matthew Wood
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
leitao, netdev, linux-kernel
On Fri, Jan 26, 2024 at 03:13:41PM -0800, Matthew Wood wrote:
> Store a formatted string for userdata that will be appended to netconsole
> messages. The string has a capacity of 4KB, as calculated by the userdatum
> entry length of 256 bytes and a max of 16 userdata entries.
>
> Update the stored netconsole_target->userdata_complete string with the new
> formatted userdata values when a userdatum is created, edited, or
> removed. Each userdata entry contains a trailing newline, which will be
> formatted as such in netconsole messages::
>
> 6.7.0-rc8-virtme,12,500,1646292204,-;test
> release=foo
> something=bar
> 6.7.0-rc8-virtme,12,500,1646292204,-;another test
> release=foo
> something=bar
>
> Enforcement of MAX_USERDATA_ITEMS is done in userdatum_make_item;
> update_userdata will not check for this case but will skip any userdata
> children over the limit of MAX_USERDATA_ITEMs.
>
> If a userdata entry/dir is created but no value is provided, that entry
> will be skipped. This is in part because update_userdata() can't be
> called in userdatum_make_item() since the item will not have been added
> to the userdata config_group children yet. To preserve the experience of
> adding an empty userdata that doesn't show up in the netconsole
> messages, purposefully skip emtpy userdata items even when
nit: empty
> update_userdata() can be called.
>
> Co-developed-by: Breno Leitao <leitao@debian.org>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
...
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH net-next v2 7/8] net: netconsole: append userdata to netconsole messages
2024-01-26 23:13 [PATCH net-next v2 0/8] netconsole: Add userdata append support Matthew Wood
` (5 preceding siblings ...)
2024-01-26 23:13 ` [PATCH net-next v2 6/8] net: netconsole: cache userdata formatted string in netconsole_target Matthew Wood
@ 2024-01-26 23:13 ` Matthew Wood
2024-01-26 23:13 ` [PATCH net-next v2 8/8] net: netconsole: append userdata to fragmented " Matthew Wood
7 siblings, 0 replies; 20+ messages in thread
From: Matthew Wood @ 2024-01-26 23:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: leitao, netdev, linux-kernel
Append userdata to outgoing unfragmented (<1000 bytes) netconsole messages.
When sending messages the userdata string is already formatted and stored
in netconsole_target->userdata_complete.
Always write the outgoing message to buf, so userdata can be appended in
a standard fashion. This is a change from only using buf when the
release needs to be prepended to the message.
Co-developed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
drivers/net/netconsole.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 73feba0b3c93..de668a0794b1 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1035,19 +1035,34 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
const char *msg_ready = msg;
const char *release;
int release_len = 0;
+ int userdata_len = 0;
+ char *userdata = NULL;
+
+#ifdef CONFIG_NETCONSOLE_DYNAMIC
+ userdata = nt->userdata_complete;
+ userdata_len = nt->userdata_length;
+#endif
if (nt->release) {
release = init_utsname()->release;
release_len = strlen(release) + 1;
}
- if (msg_len + release_len <= MAX_PRINT_CHUNK) {
+ if (msg_len + release_len + userdata_len <= MAX_PRINT_CHUNK) {
/* No fragmentation needed */
if (nt->release) {
scnprintf(buf, MAX_PRINT_CHUNK, "%s,%s", release, msg);
msg_len += release_len;
- msg_ready = buf;
+ } else {
+ memcpy(buf, msg, msg_len);
}
+
+ if (userdata)
+ msg_len += scnprintf(&buf[msg_len],
+ MAX_PRINT_CHUNK - msg_len,
+ "%s", userdata);
+
+ msg_ready = buf;
netpoll_send_udp(&nt->np, msg_ready, msg_len);
return;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH net-next v2 8/8] net: netconsole: append userdata to fragmented netconsole messages
2024-01-26 23:13 [PATCH net-next v2 0/8] netconsole: Add userdata append support Matthew Wood
` (6 preceding siblings ...)
2024-01-26 23:13 ` [PATCH net-next v2 7/8] net: netconsole: append userdata to netconsole messages Matthew Wood
@ 2024-01-26 23:13 ` Matthew Wood
7 siblings, 0 replies; 20+ messages in thread
From: Matthew Wood @ 2024-01-26 23:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: leitao, netdev, linux-kernel
Regardless of whether the original message body or formatted userdata
exceeds the MAX_PRINT_CHUNK, append userdata to the netconsole message
starting with the first chunk that has available space after writing the
body.
Co-developed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
drivers/net/netconsole.c | 50 +++++++++++++++++++++++++++++-----------
1 file changed, 37 insertions(+), 13 deletions(-)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index de668a0794b1..b33ceb110434 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1086,24 +1086,48 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
memcpy(buf + release_len, header, header_len);
header_len += release_len;
- while (offset < body_len) {
+ while (offset < body_len + userdata_len) {
int this_header = header_len;
- int this_chunk;
+ int this_offset = 0;
+ int this_chunk = 0;
this_header += scnprintf(buf + this_header,
sizeof(buf) - this_header,
- ",ncfrag=%d/%d;", offset, body_len);
-
- this_chunk = min(body_len - offset,
- MAX_PRINT_CHUNK - this_header);
- if (WARN_ON_ONCE(this_chunk <= 0))
- return;
-
- memcpy(buf + this_header, body + offset, this_chunk);
-
- netpoll_send_udp(&nt->np, buf, this_header + this_chunk);
+ ",ncfrag=%d/%d;", offset,
+ body_len + userdata_len);
+
+ /* Not all body data has been written yet */
+ if (offset < body_len) {
+ this_chunk = min(body_len - offset,
+ MAX_PRINT_CHUNK - this_header);
+ if (WARN_ON_ONCE(this_chunk <= 0))
+ return;
+ memcpy(buf + this_header, body + offset, this_chunk);
+ this_offset += this_chunk;
+ }
+ /* Body is fully written and there is pending userdata to write,
+ * append userdata in this chunk
+ */
+ if (offset + this_offset >= body_len &&
+ offset + this_offset < userdata_len + body_len) {
+ int sent_userdata = (offset + this_offset) - body_len;
+ int preceding_bytes = this_chunk + this_header;
+
+ if (WARN_ON_ONCE(sent_userdata < 0))
+ return;
+
+ this_chunk = min(userdata_len - sent_userdata,
+ MAX_PRINT_CHUNK - preceding_bytes);
+ if (WARN_ON_ONCE(this_chunk <= 0))
+ return;
+ memcpy(buf + this_header + this_offset,
+ userdata + sent_userdata,
+ this_chunk);
+ this_offset += this_chunk;
+ }
- offset += this_chunk;
+ netpoll_send_udp(&nt->np, buf, this_header + this_offset);
+ offset += this_offset;
}
}
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread