Retain error value in struct devlink_fmsg, to relieve drivers from
checking it after each call.
Note that fmsg is an in-memory builder/buffer of formatted message,
so it's not the case that half baked message was sent somewhere.
We could find following scheme in multiple drivers:
err = devlink_fmsg_obj_nest_start(fmsg);
if (err)
return err;
err = devlink_fmsg_string_pair_put(fmsg, "src", src);
if (err)
return err;
err = devlink_fmsg_something(fmsg, foo, bar);
if (err)
return err;
// and so on...
err = devlink_fmsg_obj_nest_end(fmsg);
With retaining error API that translates to:
devlink_fmsg_obj_nest_start(fmsg);
devlink_fmsg_string_pair_put(fmsg, "src", src);
devlink_fmsg_something(fmsg, foo, bar);
// and so on...
devlink_fmsg_obj_nest_end(fmsg);
What means we check error just when is time to send.
Possible error scenarios are developer error (API misuse) and memory
exhaustion, both cases are good candidates to choose readability
over fastest possible exit.
Note that this patch keeps returning errors, to allow per-driver conversion
to the new API, but those are not needed at this point already.
This commit itself is an illustration of benefits for the dev-user,
more of it will be in separate commits of the series.
Reviewed-by: Jesse Brandeburg <[email protected]>
Reviewed-by: Jiri Pirko <[email protected]>
Signed-off-by: Przemek Kitszel <[email protected]>
---
add/remove: 2/4 grow/shrink: 17/3 up/down: 462/-466 (-4)
---
net/devlink/health.c | 247 +++++++++++++------------------------------
1 file changed, 76 insertions(+), 171 deletions(-)
diff --git a/net/devlink/health.c b/net/devlink/health.c
index 51e6e81e31bb..3858a436598e 100644
--- a/net/devlink/health.c
+++ b/net/devlink/health.c
@@ -19,6 +19,7 @@ struct devlink_fmsg_item {
struct devlink_fmsg {
struct list_head item_list;
+ int err; /* first error encountered on some devlink_fmsg_XXX() call */
bool putting_binary; /* This flag forces enclosing of binary data
* in an array brackets. It forces using
* of designated API:
@@ -562,10 +563,8 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
return 0;
reporter->dump_fmsg = devlink_fmsg_alloc();
- if (!reporter->dump_fmsg) {
- err = -ENOMEM;
- return err;
- }
+ if (!reporter->dump_fmsg)
+ return -ENOMEM;
err = devlink_fmsg_obj_nest_start(reporter->dump_fmsg);
if (err)
@@ -670,43 +669,46 @@ int devlink_nl_cmd_health_reporter_recover_doit(struct sk_buff *skb,
return devlink_health_reporter_recover(reporter, NULL, info->extack);
}
-static int devlink_fmsg_nest_common(struct devlink_fmsg *fmsg,
- int attrtype)
+static void devlink_fmsg_err_if_binary(struct devlink_fmsg *fmsg)
+{
+ if (!fmsg->err && fmsg->putting_binary)
+ fmsg->err = -EINVAL;
+}
+
+static int devlink_fmsg_nest_common(struct devlink_fmsg *fmsg, int attrtype)
{
struct devlink_fmsg_item *item;
+ if (fmsg->err)
+ return fmsg->err;
+
item = kzalloc(sizeof(*item), GFP_KERNEL);
- if (!item)
- return -ENOMEM;
+ if (!item) {
+ fmsg->err = -ENOMEM;
+ return fmsg->err;
+ }
item->attrtype = attrtype;
list_add_tail(&item->list, &fmsg->item_list);
return 0;
}
int devlink_fmsg_obj_nest_start(struct devlink_fmsg *fmsg)
{
- if (fmsg->putting_binary)
- return -EINVAL;
-
+ devlink_fmsg_err_if_binary(fmsg);
return devlink_fmsg_nest_common(fmsg, DEVLINK_ATTR_FMSG_OBJ_NEST_START);
}
EXPORT_SYMBOL_GPL(devlink_fmsg_obj_nest_start);
static int devlink_fmsg_nest_end(struct devlink_fmsg *fmsg)
{
- if (fmsg->putting_binary)
- return -EINVAL;
-
+ devlink_fmsg_err_if_binary(fmsg);
return devlink_fmsg_nest_common(fmsg, DEVLINK_ATTR_FMSG_NEST_END);
}
int devlink_fmsg_obj_nest_end(struct devlink_fmsg *fmsg)
{
- if (fmsg->putting_binary)
- return -EINVAL;
-
return devlink_fmsg_nest_end(fmsg);
}
EXPORT_SYMBOL_GPL(devlink_fmsg_obj_nest_end);
@@ -717,15 +719,20 @@ static int devlink_fmsg_put_name(struct devlink_fmsg *fmsg, const char *name)
{
struct devlink_fmsg_item *item;
- if (fmsg->putting_binary)
- return -EINVAL;
+ devlink_fmsg_err_if_binary(fmsg);
+ if (fmsg->err)
+ return fmsg->err;
- if (strlen(name) + 1 > DEVLINK_FMSG_MAX_SIZE)
- return -EMSGSIZE;
+ if (strlen(name) + 1 > DEVLINK_FMSG_MAX_SIZE) {
+ fmsg->err = -EMSGSIZE;
+ return fmsg->err;
+ }
item = kzalloc(sizeof(*item) + strlen(name) + 1, GFP_KERNEL);
- if (!item)
- return -ENOMEM;
+ if (!item) {
+ fmsg->err = -ENOMEM;
+ return fmsg->err;
+ }
item->nla_type = NLA_NUL_STRING;
item->len = strlen(name) + 1;
@@ -738,68 +745,30 @@ static int devlink_fmsg_put_name(struct devlink_fmsg *fmsg, const char *name)
int devlink_fmsg_pair_nest_start(struct devlink_fmsg *fmsg, const char *name)
{
- int err;
-
- if (fmsg->putting_binary)
- return -EINVAL;
-
- err = devlink_fmsg_nest_common(fmsg, DEVLINK_ATTR_FMSG_PAIR_NEST_START);
- if (err)
- return err;
-
- err = devlink_fmsg_put_name(fmsg, name);
- if (err)
- return err;
-
- return 0;
+ devlink_fmsg_err_if_binary(fmsg);
+ devlink_fmsg_nest_common(fmsg, DEVLINK_ATTR_FMSG_PAIR_NEST_START);
+ return devlink_fmsg_put_name(fmsg, name);
}
EXPORT_SYMBOL_GPL(devlink_fmsg_pair_nest_start);
int devlink_fmsg_pair_nest_end(struct devlink_fmsg *fmsg)
{
- if (fmsg->putting_binary)
- return -EINVAL;
-
return devlink_fmsg_nest_end(fmsg);
}
EXPORT_SYMBOL_GPL(devlink_fmsg_pair_nest_end);
int devlink_fmsg_arr_pair_nest_start(struct devlink_fmsg *fmsg,
const char *name)
{
- int err;
-
- if (fmsg->putting_binary)
- return -EINVAL;
-
- err = devlink_fmsg_pair_nest_start(fmsg, name);
- if (err)
- return err;
-
- err = devlink_fmsg_nest_common(fmsg, DEVLINK_ATTR_FMSG_ARR_NEST_START);
- if (err)
- return err;
-
- return 0;
+ devlink_fmsg_pair_nest_start(fmsg, name);
+ return devlink_fmsg_nest_common(fmsg, DEVLINK_ATTR_FMSG_ARR_NEST_START);
}
EXPORT_SYMBOL_GPL(devlink_fmsg_arr_pair_nest_start);
int devlink_fmsg_arr_pair_nest_end(struct devlink_fmsg *fmsg)
{
- int err;
-
- if (fmsg->putting_binary)
- return -EINVAL;
-
- err = devlink_fmsg_nest_end(fmsg);
- if (err)
- return err;
-
- err = devlink_fmsg_nest_end(fmsg);
- if (err)
- return err;
-
- return 0;
+ devlink_fmsg_nest_end(fmsg);
+ return devlink_fmsg_nest_end(fmsg);
}
EXPORT_SYMBOL_GPL(devlink_fmsg_arr_pair_nest_end);
@@ -813,14 +782,19 @@ int devlink_fmsg_binary_pair_nest_start(struct devlink_fmsg *fmsg,
return err;
fmsg->putting_binary = true;
- return err;
+ return 0;
}
EXPORT_SYMBOL_GPL(devlink_fmsg_binary_pair_nest_start);
int devlink_fmsg_binary_pair_nest_end(struct devlink_fmsg *fmsg)
{
- if (!fmsg->putting_binary)
- return -EINVAL;
+ if (fmsg->err)
+ return fmsg->err;
+
+ if (!fmsg->putting_binary) {
+ fmsg->err = -EINVAL;
+ return fmsg->err;
+ }
fmsg->putting_binary = false;
return devlink_fmsg_arr_pair_nest_end(fmsg);
@@ -833,12 +807,16 @@ static int devlink_fmsg_put_value(struct devlink_fmsg *fmsg,
{
struct devlink_fmsg_item *item;
- if (value_len > DEVLINK_FMSG_MAX_SIZE)
- return -EMSGSIZE;
+ if (value_len > DEVLINK_FMSG_MAX_SIZE) {
+ fmsg->err = -EMSGSIZE;
+ return fmsg->err;
+ }
item = kzalloc(sizeof(*item) + value_len, GFP_KERNEL);
- if (!item)
- return -ENOMEM;
+ if (!item) {
+ fmsg->err = -ENOMEM;
+ return fmsg->err;
+ }
item->nla_type = value_nla_type;
item->len = value_len;
@@ -851,42 +829,32 @@ static int devlink_fmsg_put_value(struct devlink_fmsg *fmsg,
static int devlink_fmsg_bool_put(struct devlink_fmsg *fmsg, bool value)
{
- if (fmsg->putting_binary)
- return -EINVAL;
-
+ devlink_fmsg_err_if_binary(fmsg);
return devlink_fmsg_put_value(fmsg, &value, sizeof(value), NLA_FLAG);
}
static int devlink_fmsg_u8_put(struct devlink_fmsg *fmsg, u8 value)
{
- if (fmsg->putting_binary)
- return -EINVAL;
-
+ devlink_fmsg_err_if_binary(fmsg);
return devlink_fmsg_put_value(fmsg, &value, sizeof(value), NLA_U8);
}
int devlink_fmsg_u32_put(struct devlink_fmsg *fmsg, u32 value)
{
- if (fmsg->putting_binary)
- return -EINVAL;
-
+ devlink_fmsg_err_if_binary(fmsg);
return devlink_fmsg_put_value(fmsg, &value, sizeof(value), NLA_U32);
}
EXPORT_SYMBOL_GPL(devlink_fmsg_u32_put);
static int devlink_fmsg_u64_put(struct devlink_fmsg *fmsg, u64 value)
{
- if (fmsg->putting_binary)
- return -EINVAL;
-
+ devlink_fmsg_err_if_binary(fmsg);
return devlink_fmsg_put_value(fmsg, &value, sizeof(value), NLA_U64);
}
int devlink_fmsg_string_put(struct devlink_fmsg *fmsg, const char *value)
{
- if (fmsg->putting_binary)
- return -EINVAL;
-
+ devlink_fmsg_err_if_binary(fmsg);
return devlink_fmsg_put_value(fmsg, value, strlen(value) + 1,
NLA_NUL_STRING);
}
@@ -905,113 +873,52 @@ EXPORT_SYMBOL_GPL(devlink_fmsg_binary_put);
int devlink_fmsg_bool_pair_put(struct devlink_fmsg *fmsg, const char *name,
bool value)
{
- int err;
-
- err = devlink_fmsg_pair_nest_start(fmsg, name);
- if (err)
- return err;
-
- err = devlink_fmsg_bool_put(fmsg, value);
- if (err)
- return err;
-
- err = devlink_fmsg_pair_nest_end(fmsg);
- if (err)
- return err;
-
- return 0;
+ devlink_fmsg_pair_nest_start(fmsg, name);
+ devlink_fmsg_bool_put(fmsg, value);
+ return devlink_fmsg_pair_nest_end(fmsg);
}
EXPORT_SYMBOL_GPL(devlink_fmsg_bool_pair_put);
int devlink_fmsg_u8_pair_put(struct devlink_fmsg *fmsg, const char *name,
u8 value)
{
- int err;
-
- err = devlink_fmsg_pair_nest_start(fmsg, name);
- if (err)
- return err;
-
- err = devlink_fmsg_u8_put(fmsg, value);
- if (err)
- return err;
-
- err = devlink_fmsg_pair_nest_end(fmsg);
- if (err)
- return err;
-
- return 0;
+ devlink_fmsg_pair_nest_start(fmsg, name);
+ devlink_fmsg_u8_put(fmsg, value);
+ return devlink_fmsg_pair_nest_end(fmsg);
}
EXPORT_SYMBOL_GPL(devlink_fmsg_u8_pair_put);
int devlink_fmsg_u32_pair_put(struct devlink_fmsg *fmsg, const char *name,
u32 value)
{
- int err;
-
- err = devlink_fmsg_pair_nest_start(fmsg, name);
- if (err)
- return err;
-
- err = devlink_fmsg_u32_put(fmsg, value);
- if (err)
- return err;
-
- err = devlink_fmsg_pair_nest_end(fmsg);
- if (err)
- return err;
-
- return 0;
+ devlink_fmsg_pair_nest_start(fmsg, name);
+ devlink_fmsg_u32_put(fmsg, value);
+ return devlink_fmsg_pair_nest_end(fmsg);
}
EXPORT_SYMBOL_GPL(devlink_fmsg_u32_pair_put);
int devlink_fmsg_u64_pair_put(struct devlink_fmsg *fmsg, const char *name,
u64 value)
{
- int err;
-
- err = devlink_fmsg_pair_nest_start(fmsg, name);
- if (err)
- return err;
-
- err = devlink_fmsg_u64_put(fmsg, value);
- if (err)
- return err;
-
- err = devlink_fmsg_pair_nest_end(fmsg);
- if (err)
- return err;
-
- return 0;
+ devlink_fmsg_pair_nest_start(fmsg, name);
+ devlink_fmsg_u64_put(fmsg, value);
+ return devlink_fmsg_pair_nest_end(fmsg);
}
EXPORT_SYMBOL_GPL(devlink_fmsg_u64_pair_put);
int devlink_fmsg_string_pair_put(struct devlink_fmsg *fmsg, const char *name,
const char *value)
{
- int err;
-
- err = devlink_fmsg_pair_nest_start(fmsg, name);
- if (err)
- return err;
-
- err = devlink_fmsg_string_put(fmsg, value);
- if (err)
- return err;
-
- err = devlink_fmsg_pair_nest_end(fmsg);
- if (err)
- return err;
-
- return 0;
+ devlink_fmsg_pair_nest_start(fmsg, name);
+ devlink_fmsg_string_put(fmsg, value);
+ return devlink_fmsg_pair_nest_end(fmsg);
}
EXPORT_SYMBOL_GPL(devlink_fmsg_string_pair_put);
int devlink_fmsg_binary_pair_put(struct devlink_fmsg *fmsg, const char *name,
const void *value, u32 value_len)
{
u32 data_size;
- int end_err;
u32 offset;
int err;
@@ -1027,14 +934,12 @@ int devlink_fmsg_binary_pair_put(struct devlink_fmsg *fmsg, const char *name,
if (err)
break;
/* Exit from loop with a break (instead of
- * return) to make sure putting_binary is turned off in
- * devlink_fmsg_binary_pair_nest_end
+ * return) to make sure putting_binary is turned off
*/
}
- end_err = devlink_fmsg_binary_pair_nest_end(fmsg);
- if (end_err)
- err = end_err;
+ err = devlink_fmsg_binary_pair_nest_end(fmsg);
+ fmsg->putting_binary = false;
return err;
}
--
2.38.1
On Wed, Oct 18, 2023 at 10:26:37PM +0200, Przemek Kitszel wrote:
> Retain error value in struct devlink_fmsg, to relieve drivers from
> checking it after each call.
> Note that fmsg is an in-memory builder/buffer of formatted message,
> so it's not the case that half baked message was sent somewhere.
>
> We could find following scheme in multiple drivers:
> err = devlink_fmsg_obj_nest_start(fmsg);
> if (err)
> return err;
> err = devlink_fmsg_string_pair_put(fmsg, "src", src);
> if (err)
> return err;
> err = devlink_fmsg_something(fmsg, foo, bar);
> if (err)
> return err;
> // and so on...
> err = devlink_fmsg_obj_nest_end(fmsg);
>
> With retaining error API that translates to:
> devlink_fmsg_obj_nest_start(fmsg);
> devlink_fmsg_string_pair_put(fmsg, "src", src);
> devlink_fmsg_something(fmsg, foo, bar);
> // and so on...
> devlink_fmsg_obj_nest_end(fmsg);
>
> What means we check error just when is time to send.
>
> Possible error scenarios are developer error (API misuse) and memory
> exhaustion, both cases are good candidates to choose readability
> over fastest possible exit.
>
> Note that this patch keeps returning errors, to allow per-driver conversion
> to the new API, but those are not needed at this point already.
>
> This commit itself is an illustration of benefits for the dev-user,
> more of it will be in separate commits of the series.
>
> Reviewed-by: Jesse Brandeburg <[email protected]>
> Reviewed-by: Jiri Pirko <[email protected]>
> Signed-off-by: Przemek Kitszel <[email protected]>
...
> @@ -1027,14 +934,12 @@ int devlink_fmsg_binary_pair_put(struct devlink_fmsg *fmsg, const char *name,
Hi Przemek,
The line before this hunk is:
err = devlink_fmsg_binary_put(fmsg, value + offset, data_size);
And, as of this patch, the implementation of
devlink_fmsg_binary_pair_nest_start() looks like this:
int devlink_fmsg_binary_put(struct devlink_fmsg *fmsg, const void *value,
u16 value_len)
{
if (!fmsg->putting_binary)
return -EINVAL;
return devlink_fmsg_put_value(fmsg, value, value_len, NLA_BINARY);
}
Which may return an error, if the if condition is met, without setting
fmsg->err.
> if (err)
> break;
> /* Exit from loop with a break (instead of
> - * return) to make sure putting_binary is turned off in
> - * devlink_fmsg_binary_pair_nest_end
> + * return) to make sure putting_binary is turned off
> */
> }
>
> - end_err = devlink_fmsg_binary_pair_nest_end(fmsg);
> - if (end_err)
> - err = end_err;
Prior to this patch, the value of err from the loop above was preserved,
unless devlink_fmsg_binary_pair_nest_end generated an error.
> + err = devlink_fmsg_binary_pair_nest_end(fmsg);
But now it looks like this is only the case if fmsg->err corresponds to err
when the loop was exited.
Or in other words, the err returned by devlink_fmsg_binary_put()
is not propagated to the caller if !fmsg->putting_binary.
If so, is this intentional?
> + fmsg->putting_binary = false;
>
> return err;
> }
> --
> 2.38.1
>
On 10/19/23 15:00, Simon Horman wrote:
> On Wed, Oct 18, 2023 at 10:26:37PM +0200, Przemek Kitszel wrote:
>> Retain error value in struct devlink_fmsg, to relieve drivers from
>> checking it after each call.
>> Note that fmsg is an in-memory builder/buffer of formatted message,
>> so it's not the case that half baked message was sent somewhere.
>>
>> We could find following scheme in multiple drivers:
>> err = devlink_fmsg_obj_nest_start(fmsg);
>> if (err)
>> return err;
>> err = devlink_fmsg_string_pair_put(fmsg, "src", src);
>> if (err)
>> return err;
>> err = devlink_fmsg_something(fmsg, foo, bar);
>> if (err)
>> return err;
>> // and so on...
>> err = devlink_fmsg_obj_nest_end(fmsg);
>>
>> With retaining error API that translates to:
>> devlink_fmsg_obj_nest_start(fmsg);
>> devlink_fmsg_string_pair_put(fmsg, "src", src);
>> devlink_fmsg_something(fmsg, foo, bar);
>> // and so on...
>> devlink_fmsg_obj_nest_end(fmsg);
>>
>> What means we check error just when is time to send.
>>
>> Possible error scenarios are developer error (API misuse) and memory
>> exhaustion, both cases are good candidates to choose readability
>> over fastest possible exit.
>>
>> Note that this patch keeps returning errors, to allow per-driver conversion
>> to the new API, but those are not needed at this point already.
>>
>> This commit itself is an illustration of benefits for the dev-user,
>> more of it will be in separate commits of the series.
>>
>> Reviewed-by: Jesse Brandeburg <[email protected]>
>> Reviewed-by: Jiri Pirko <[email protected]>
>> Signed-off-by: Przemek Kitszel <[email protected]>
>
> ...
>
>> @@ -1027,14 +934,12 @@ int devlink_fmsg_binary_pair_put(struct devlink_fmsg *fmsg, const char *name,
>
> Hi Przemek,
>
> The line before this hunk is:
>
> err = devlink_fmsg_binary_put(fmsg, value + offset, data_size);
>
> And, as of this patch, the implementation of
> devlink_fmsg_binary_pair_nest_start() looks like this:
>
> int devlink_fmsg_binary_put(struct devlink_fmsg *fmsg, const void *value,
> u16 value_len)
> {
> if (!fmsg->putting_binary)
> return -EINVAL;
>
> return devlink_fmsg_put_value(fmsg, value, value_len, NLA_BINARY);
> }
>
> Which may return an error, if the if condition is met, without setting
> fmsg->err.
>
>> if (err)
>> break;
>> /* Exit from loop with a break (instead of
>> - * return) to make sure putting_binary is turned off in
>> - * devlink_fmsg_binary_pair_nest_end
>> + * return) to make sure putting_binary is turned off
>> */
>> }
>>
>> - end_err = devlink_fmsg_binary_pair_nest_end(fmsg);
>> - if (end_err)
>> - err = end_err;
>
> Prior to this patch, the value of err from the loop above was preserved,
> unless devlink_fmsg_binary_pair_nest_end generated an error.
>
>> + err = devlink_fmsg_binary_pair_nest_end(fmsg);
>
> But now it looks like this is only the case if fmsg->err corresponds to err
> when the loop was exited.
>
> Or in other words, the err returned by devlink_fmsg_binary_put()
> is not propagated to the caller if !fmsg->putting_binary.
>
> If so, is this intentional?
In scope of devlink_fmsg_binary_pair_put() all it's fine, and perhaps
that lead me into thinking that it's fine in *general*, which is not,
as devlink_fmsg_binary_put() is exported (so it's return value should
be preserved).
On the other note, it's called in two places (one of which was just
covered, and the second place is mlx5e_health_rsc_fmsg_binary(), which
really just mimics what we have here. So this is also fine in practice.
Last patch of the series fixes it for general case, so barring
pathological cherry-picks or reverts we are safe.
Do you want a v4 with that fixed?
Other thing is that this function could be changed into some internal
helper without export.
>
>> + fmsg->putting_binary = false;
>>
>> return err;
>> }
>> --
>> 2.38.1
>>
>