On Wed. 1 Feb 2023 at 17:58, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote: > On 01.02.2023 14:49:23, Vincent Mailhol wrote: > > > I've converted the existing netdev_err() NL_SET_ERR_MSG_FMT(). This > > > means the error message is transported via netlink to user space and > > > printed by the "ip" tool. > > > > I was not aware of this NL_SET_ERR_MSG_FMT() thing, and let me say > > that I really like that solution! > > > > > | # ip link set flexcan0 txqueuelen 10 type can bitrate 2200000 > > > | Warning: bitrate error 2.5%. > > > > The Linux coding style says: > > > > Kernel messages do not have to be terminated with a period. > > > > Ref: https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages > > > > However, I am not sure if this also applies to the ip tool. > > I think the period is added by "ip". Ack. All good then! > > > | > > > | # ip link set flexcan0 txqueuelen 10 type can bitrate 22000000 > > > | Error: bitrate error 80.5% too high. > > > > > > This is the error message for the SJW check: > > > > > > | # ip link set flexcan0 txqueuelen 10 type can bitrate 500000 sjw 3 > > > | Error: SJW 3 bigger than phase_seg1 6 and/or phase_seg2 2. > > > > At that point in the code, I assume that sjw was already validated > > against sjw_max. While not impossible, I think that having sjw bigger > > than both phase_seg1 and phase_seg2 at the same time is uncommon. I > > suggest to split the error message in two and only print the relevant > > one: > > > > Error: SJW 3 bigger than phase_seg1 x > > Error: SJW 3 bigger than phase_seg2 x > > > > If the user still manages to violate both, only the phase_seg1 error > > message is displayed. > > Makes sense. > > > > Maybe add a '=' between the phase_seg and the actual number: > > > > > > | Error: SJW 3 bigger than phase_seg1=6 and/or phase_seg2=2. > > > > If you ask me my preferences, I will go with column: > > > > Error: SJW: 3 bigger than phase_seg1: 6 and/or phase_seg2: 2. > > > > But I will not complain if you pick anything else. > > Period looks good. > > What about the error value? Always return -EINVAL instead of a mix of > -EINVAL and -ERANGE? Looking at the comments from uapi/asm-generic/errno-base.h: #define EINVAL 22 /* Invalid argument */ #define ERANGE 34 /* Math result not representable */ We are not dealing with some non-representable math results, so -ERANGE is technically incorrect. I did suggest -ERANGE in the past because it looks natural to me: we define a min and a max, i.e. its a range. But the doc tells me I am wrong. Naming the error -ENAN (Error Not A Number) would have been more explicit, but we are not going to rewrite UAPI error definitions. Go with -EINVAL This answer also applies to your next message and to everywhere else in netlink, I guess. Yours sincerely, Vincent Mailhol
