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 <jesse.brandeburg@xxxxxxxxx>
Reviewed-by: Jiri Pirko <jiri@xxxxxxxxxx>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@xxxxxxxxx>
...
@@ -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