| Message ID | 20230320132523.3203254-2-shayagr@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add tx push buf len param to ethtool | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 5054 this patch: 5054 |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/build_clang | success | Errors and warnings before: 985 this patch: 985 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 5263 this patch: 5263 |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, 2023-03-20 at 15:25 +0200, Shay Agroskin wrote: > Similar to NL_SET_ERR_MSG_FMT, add a macro which sets netlink policy > error message with a format string. > > Signed-off-by: Shay Agroskin <shayagr@amazon.com> > --- > include/linux/netlink.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/include/linux/netlink.h b/include/linux/netlink.h > index 3e8743252167..2ca76ec1fc33 100644 > --- a/include/linux/netlink.h > +++ b/include/linux/netlink.h > @@ -161,9 +161,22 @@ struct netlink_ext_ack { > } \ > } while (0) > > +#define NL_SET_ERR_MSG_ATTR_POL_FMT(extack, attr, pol, fmt, args...) do { \ > + struct netlink_ext_ack *__extack = (extack); \ > + \ > + if (__extack) { \ > + NL_SET_ERR_MSG_FMT(extack, fmt, ##args); \ You should use '__extack' even above, to avoid multiple evaluation of the 'extack' expression. Side note: you dropped the acked-by/revied-by tags collected on previous iterations, you could and should have retained them for the unmodified patches. Thanks, Paolo
Paolo Abeni <pabeni@redhat.com> writes: > CAUTION: This email originated from outside of the > organization. Do not click links or open attachments unless you > can confirm the sender and know the content is safe. > > > > On Mon, 2023-03-20 at 15:25 +0200, Shay Agroskin wrote: >> Similar to NL_SET_ERR_MSG_FMT, add a macro which sets netlink >> policy >> error message with a format string. >> >> Signed-off-by: Shay Agroskin <shayagr@amazon.com> >> --- >> include/linux/netlink.h | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/include/linux/netlink.h b/include/linux/netlink.h >> index 3e8743252167..2ca76ec1fc33 100644 >> --- a/include/linux/netlink.h >> +++ b/include/linux/netlink.h >> @@ -161,9 +161,22 @@ struct netlink_ext_ack { >> } \ >> } while (0) >> >> +#define NL_SET_ERR_MSG_ATTR_POL_FMT(extack, attr, pol, fmt, >> args...) do { \ >> + struct netlink_ext_ack *__extack = (extack); >> \ >> + >> \ >> + if (__extack) { >> \ >> + NL_SET_ERR_MSG_FMT(extack, fmt, ##args); >> \ Thanks for reviewing the patch > > You should use '__extack' even above, to avoid multiple > evaluation of > the 'extack' expression. I've got to admit that I don't understand the cost of such evaluations. I thought that it was added to help readers of the source code to understand what is the type of this attribute and have a better warning message when the wrong variable is passed (kind of typing in Python which isn't strictly needed). What cost is there for casting a pointer ? > > Side note: you dropped the acked-by/revied-by tags collected on > previous iterations, you could and should have retained them for > the > unmodified patches. Yap that's an oversight by my side. Forgot to do it in the last patchset. I'll make sure to do it in the next patchset > > Thanks, > > Paolo
On Wed, 22 Mar 2023 14:39:49 +0200 Shay Agroskin wrote: > > You should use '__extack' even above, to avoid multiple > > evaluation of > > the 'extack' expression. > > I've got to admit that I don't understand the cost of such > evaluations. I thought that it was added to help readers of the > source code to understand what is the type of this attribute and > have a better warning message when the wrong variable is passed > (kind of typing in Python which isn't strictly needed). > What cost is there for casting a pointer ? It's not about the cost, the macros are unfolded by the preprocessor, in the unlikely case someone passes extack++ as an argument using it twice inside the body of the macro will increment the value twice. #define MACRO(arg) function_bla(arg, arg) // use arg twice int a = 1; MACRO(a++); print(a); // should print 2, will print 3
Jakub Kicinski <kuba@kernel.org> writes: > On Wed, 22 Mar 2023 14:39:49 +0200 Shay Agroskin wrote: >> > You should use '__extack' even above, to avoid multiple >> > evaluation of >> > the 'extack' expression. >> >> I've got to admit that I don't understand the cost of such >> evaluations. I thought that it was added to help readers of the >> source code to understand what is the type of this attribute >> and >> have a better warning message when the wrong variable is passed >> (kind of typing in Python which isn't strictly needed). >> What cost is there for casting a pointer ? > > It's not about the cost, the macros are unfolded by the > preprocessor, > in the unlikely case someone passes extack++ as an argument > using it > twice inside the body of the macro will increment the value > twice. > > #define MACRO(arg) function_bla(arg, arg) // use arg twice > > int a = 1; > MACRO(a++); > print(a); // should print 2, will print 3 Thanks for explaining, that's quite cool how something like this can cause a very hard to find bug. Couldn't find a way to avoid both code duplication and evaluating extact only once \= Submitted a new patchset with the modified version of this macro. Also added Reviewed-by tags where appropriate
On Thu, 23 Mar 2023 18:38:59 +0200 Shay Agroskin wrote: > Couldn't find a way to avoid both code duplication and evaluating > extact only once \= Submitted a new patchset with the modified > version of this macro. That's why we have the local variable called __extack, that we *can* use multiple times, because it's a separate variable, (extack) is evaluated only once, to initialize it... We don't need to copy the string formatting, unless I'm missing something. Paolo was just asking for: - NL_SET_ERR_MSG_FMT(extack, fmt, ##args); + NL_SET_ERR_MSG_FMT(__extack, fmt, ##args); that's it.
Jakub Kicinski <kuba@kernel.org> writes: > CAUTION: This email originated from outside of the > organization. Do not click links or open attachments unless you > can confirm the sender and know the content is safe. > > > > On Thu, 23 Mar 2023 18:38:59 +0200 Shay Agroskin wrote: >> Couldn't find a way to avoid both code duplication and >> evaluating >> extact only once \= Submitted a new patchset with the modified >> version of this macro. > > That's why we have the local variable called __extack, that we > *can* > use multiple times, because it's a separate variable, (extack) > is > evaluated only once, to initialize it... > > We don't need to copy the string formatting, unless I'm missing > something. Paolo was just asking for: > > - NL_SET_ERR_MSG_FMT(extack, fmt, ##args); > + NL_SET_ERR_MSG_FMT(__extack, fmt, ##args); > > that's it. Oh shoot... That makes much more sense than my solution .... Thanks
Jakub Kicinski <kuba@kernel.org> writes: > CAUTION: This email originated from outside of the > organization. Do not click links or open attachments unless you > can confirm the sender and know the content is safe. > > > > On Thu, 23 Mar 2023 18:38:59 +0200 Shay Agroskin wrote: >> Couldn't find a way to avoid both code duplication and >> evaluating >> extact only once \= Submitted a new patchset with the modified >> version of this macro. > > That's why we have the local variable called __extack, that we > *can* > use multiple times, because it's a separate variable, (extack) > is > evaluated only once, to initialize it... > > We don't need to copy the string formatting, unless I'm missing > something. Paolo was just asking for: There is an issue with shadowing __extack by NL_SET_ERR_MSG_FMT causing the changes to __extack not to be propagated back to the caller. I'm not that big of an expert in C but changing __extack -> _extack fixes the shadowing issue. Might not be the most robust solution, though it might suffice for this use case. > > - NL_SET_ERR_MSG_FMT(extack, fmt, ##args); > + NL_SET_ERR_MSG_FMT(__extack, fmt, ##args); > > that's it.
On Thu, 23 Mar 2023 21:44:52 +0200 Shay Agroskin wrote: > > That's why we have the local variable called __extack, that we > > *can* > > use multiple times, because it's a separate variable, (extack) > > is > > evaluated only once, to initialize it... > > > > We don't need to copy the string formatting, unless I'm missing > > something. Paolo was just asking for: > > There is an issue with shadowing __extack by NL_SET_ERR_MSG_FMT > causing the changes to __extack not to be propagated back to the > caller. > I'm not that big of an expert in C but changing __extack -> > _extack fixes the shadowing issue. > > Might not be the most robust solution, though it might suffice for > this use case. TBH the hierarchy should be the other way around, NL_SET_ERR_MSG_FMT() should be converted to be: #define NL_SET_ERR_MSG_FMT(extack, attr, msg, args...) \ NL_SET_ERR_MSG_ATTR_POL_FMT(extack, NULL, NULL, msg, ##args) and that'd fix the shadowing, right?
Jakub Kicinski <kuba@kernel.org> writes: > CAUTION: This email originated from outside of the > organization. Do not click links or open attachments unless you > can confirm the sender and know the content is safe. > > > > On Thu, 23 Mar 2023 21:44:52 +0200 Shay Agroskin wrote: >> > That's why we have the local variable called __extack, that >> > we >> > *can* >> > use multiple times, because it's a separate variable, >> > (extack) >> > is >> > evaluated only once, to initialize it... >> > >> > We don't need to copy the string formatting, unless I'm >> > missing >> > something. Paolo was just asking for: >> >> There is an issue with shadowing __extack by NL_SET_ERR_MSG_FMT >> causing the changes to __extack not to be propagated back to >> the >> caller. >> I'm not that big of an expert in C but changing __extack -> >> _extack fixes the shadowing issue. >> >> Might not be the most robust solution, though it might suffice >> for >> this use case. > > TBH the hierarchy should be the other way around, > NL_SET_ERR_MSG_FMT() > should be converted to be: > > #define NL_SET_ERR_MSG_FMT(extack, attr, msg, args...) \ > NL_SET_ERR_MSG_ATTR_POL_FMT(extack, NULL, NULL, msg, > ##args) > > and that'd fix the shadowing, right? Well ... It will but it will contradict the current order of calls as I see it. NL_SET_ERR_MSG_FMT_MOD calls NL_SET_ERR_MSG_FMT which can be described as 'the former extends the latter'. On the other hand NL_SET_ERR_MSG_ATTR_POL implements the message setting by itself and doesn't use NL_SET_ERR_MSG to set the message. So it seems like we already have two approaches for macro ordering here and following your suggestion would create another type of call direction of 'the former shrinks the latter by setting to NULL its attributes'. Overall given the nature of C macros and the weird issues arise by shadowing variables (ending up for some reason in not modifying the pointer at all..) I'd say I mostly prefer V7 version which re-implements the message setting and avoids creating such very hard to find issues later. Then again I'd follow your implementation suggestion if you still prefer it (also I can modify the macro NL_SET_ERR_MSG to call NL_SET_ERR_MSG_ATTR_POL to be consistent with the other change) Shay
On Sat, 25 Mar 2023 16:49:34 +0300 Shay Agroskin wrote: > > TBH the hierarchy should be the other way around, > > NL_SET_ERR_MSG_FMT() > > should be converted to be: > > > > #define NL_SET_ERR_MSG_FMT(extack, attr, msg, args...) \ > > NL_SET_ERR_MSG_ATTR_POL_FMT(extack, NULL, NULL, msg, > > ##args) > > > > and that'd fix the shadowing, right? > > Well ... It will but it will contradict the current order of calls > as I see it. > > NL_SET_ERR_MSG_FMT_MOD calls NL_SET_ERR_MSG_FMT which can be > described as 'the former extends the latter'. > > On the other hand NL_SET_ERR_MSG_ATTR_POL implements the message > setting by itself and doesn't use NL_SET_ERR_MSG to set the > message. > > So it seems like we already have two approaches for macro ordering > here and following your suggestion would create another type of > call direction of 'the former shrinks the latter by setting to > NULL its attributes'. > Overall given the nature of C macros and the weird issues arise by > shadowing variables (ending up for some reason in not modifying > the pointer at all..) I'd say I mostly prefer V7 version which > re-implements the message setting and avoids creating such very > hard to find issues later. > > Then again I'd follow your implementation suggestion if you still > prefer it (also I can modify the macro NL_SET_ERR_MSG to call > NL_SET_ERR_MSG_ATTR_POL to be consistent with the other change) Alright, it doesn't matter all that much.
diff --git a/include/linux/netlink.h b/include/linux/netlink.h index 3e8743252167..2ca76ec1fc33 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -161,9 +161,22 @@ struct netlink_ext_ack { } \ } while (0) +#define NL_SET_ERR_MSG_ATTR_POL_FMT(extack, attr, pol, fmt, args...) do { \ + struct netlink_ext_ack *__extack = (extack); \ + \ + if (__extack) { \ + NL_SET_ERR_MSG_FMT(extack, fmt, ##args); \ + __extack->bad_attr = (attr); \ + __extack->policy = (pol); \ + } \ +} while (0) + #define NL_SET_ERR_MSG_ATTR(extack, attr, msg) \ NL_SET_ERR_MSG_ATTR_POL(extack, attr, NULL, msg) +#define NL_SET_ERR_MSG_ATTR_FMT(extack, attr, msg, args...) \ + NL_SET_ERR_MSG_ATTR_POL_FMT(extack, attr, NULL, msg, ##args) + #define NL_SET_ERR_ATTR_MISS(extack, nest, type) do { \ struct netlink_ext_ack *__extack = (extack); \ \
Similar to NL_SET_ERR_MSG_FMT, add a macro which sets netlink policy error message with a format string. Signed-off-by: Shay Agroskin <shayagr@amazon.com> --- include/linux/netlink.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)