Messages in this thread | | | Date | Thu, 19 Oct 2023 23:49:29 +0200 | Subject | Re: [PATCH net-next v3 01/11] devlink: retain error in struct devlink_fmsg | From | Przemek Kitszel <> |
| |
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 <jesse.brandeburg@intel.com> >> Reviewed-by: Jiri Pirko <jiri@nvidia.com> >> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > ... > >> @@ -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 >> >
| |