On 2/7/23 15:10, Lucero Palau, Alejandro wrote:
> On 2/7/23 14:58, Jiri Pirko wrote:
>> Tue, Feb 07, 2023 at 03:42:45PM CET, alejandro.lucero-palau@xxxxxxx wrote:
>>> On 2/2/23 11:58, Jiri Pirko wrote:
>>>> Thu, Feb 02, 2023 at 12:14:17PM CET, alejandro.lucero-palau@xxxxxxx wrote:
>>>>> From: Alejandro Lucero <alejandro.lucero-palau@xxxxxxx>
>>>>>
>>>>> Support for devlink info command.
>>>> You are quite brief for couple hundred line patch. Care to shed some
>>>> more details for the reader? Also, use imperative mood (applies to the
>>>> rest of the pathes)
>>>>
>>>> [...]
>>>>
>>> OK. I'll be more talkative and imperative here.
>>>
>>>>> +static int efx_devlink_info_get(struct devlink *devlink,
>>>>> + struct devlink_info_req *req,
>>>>> + struct netlink_ext_ack *extack)
>>>>> +{
>>>>> + struct efx_devlink *devlink_private = devlink_priv(devlink);
>>>>> + struct efx_nic *efx = devlink_private->efx;
>>>>> + char msg[NETLINK_MAX_FMTMSG_LEN];
>>>>> + int errors_reported = 0;
>>>>> + int rc;
>>>>> +
>>>>> + /* Several different MCDI commands are used. We report first error
>>>>> + * through extack along with total number of errors. Specific error
>>>>> + * information via system messages.
>>>>> + */
>>>>> + rc = efx_devlink_info_board_cfg(efx, req);
>>>>> + if (rc) {
>>>>> + sprintf(msg, "Getting board info failed");
>>>>> + errors_reported++;
>>>>> + }
>>>>> + rc = efx_devlink_info_stored_versions(efx, req);
>>>>> + if (rc) {
>>>>> + if (!errors_reported)
>>>>> + sprintf(msg, "Getting stored versions failed");
>>>>> + errors_reported += rc;
>>>>> + }
>>>>> + rc = efx_devlink_info_running_versions(efx, req);
>>>>> + if (rc) {
>>>>> + if (!errors_reported)
>>>>> + sprintf(msg, "Getting board info failed");
>>>>> + errors_reported++;
>>>> Under which circumstances any of the errors above happen? Is it a common
>>>> thing? Or is it result of some fatal event?
>>> They are not common at all. If any of those happen, it is a bad sign,
>>> and it is more than likely there are more than one because something is
>>> not working properly. That is the reason I only report first error found
>>> plus the total number of errors detected.
>>>
>>>
>>>> You treat it like it is quite common, which seems very odd to me.
>>>> If they are rare, just return error right away to the caller.
>>> Well, that is done now. And as I say, I'm not reporting all but just the
>>> first one, mainly because the buffer limitation with NETLINK_MAX_FMTMSG_LEN.
>>>
>>> If errors trigger, a more complete information will appear in system
>>> messages, so that is the reason with:
>>>
>>> + NL_SET_ERR_MSG_FMT(extack,
>>> + "%s. %d total errors. Check system messages",
>>> + msg, errors_reported);
>>>
>>> I guess you are concerned with the extack report being overwhelmed, but
>>> I do not think that is the case.
>> No, I'm wondering why you just don't put error message into exack and
>> return -ESOMEERROR right away.
> Well, I thought the idea was to give more information to user space
> about the problem.
>
> Previous patchsets were not reporting any error nor error information
> through extack. Now we have both.
Just trying to make more sense of this.
Because that limit with NETLINK_MAX_FMTMSG_LEN, what I think is big
enough, some control needs to be taken about what to report. It could be
just to write the buffer with the last error and report that last one
only, with no need of keeping total errors count. But I felt once we
handle any error, reporting that extra info about the total errors
detected should not be a problem at all, even if it is an unlikely
situation.
BTW, I said we were reporting both, the error and the extack error
message, but I've realized the function was not returning any error but
always 0, so I'll fix that.
>>>>> + }
>>>>> +
>>>>> + if (errors_reported)
>>>>> + NL_SET_ERR_MSG_FMT(extack,
>>>>> + "%s. %d total errors. Check system messages",
>>>>> + msg, errors_reported);
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> static const struct devlink_ops sfc_devlink_ops = {
>>>>> + .info_get = efx_devlink_info_get,
>>>>> };
>>>> [...]
