On Thu, 2024-10-24 at 17:17 +0200, Sabrina Dubroca wrote: > 2024-10-24, 16:52:00 +0200, Johannes Berg wrote: > > On Thu, 2024-10-24 at 16:48 +0200, Sabrina Dubroca wrote: > > > > > > If nla_get_*_default was a macro (generating an "attr ? getter : > > > default" expression) you wouldn't have that problem I think, > > > > Hmm. Perhaps. In the conditional operator (?:) they're subject to > > integer promotion though > > Is it? > > #define nla_get_u16_default(attr, d) (attr ? nla_get_u16(attr) : d) > int v = nla_get_u16_default(NULL, -1); > > seems to put the correct value into v. > (but -ENOFOOD and -ELOWCOFFEE here, so I don't trust this quick test > much :)) I'm probably -ELOWBLOODSUGAR right now ;-) But yeah, I couldn't yet construct a scenario where it mattered, but I' also couldn't convince myself that there isn't one. Maybe too much inspired by the enum compare warnings: https://lore.kernel.org/linux-wireless/20241018151841.3821671-1-arnd@xxxxxxxxxx/ > > , I wonder if that could cause some subtle issue > > too especially if nla_get_u*() is used with signed variables? > > The issue in that example is pretty subtle and I'm fairly sure people > are going to mess up :/ I didn't think it was that bad, but it's well possible that my calibration for "subtle" is way off ;-) > But I'm not attached to that macro I just suggested, it's just a > thought. Sure. > > > but you > > > couldn't nicely generate all the helpers with MAKE_NLA_GET_DEFAULT > > > anymore. > > > > Right, that too. > > > > I think it's probably better to just review them, and only commit the > > obvious ones originally? > > Well, this one looked reasonable too. I'm not convinced reviewers are > going to catch those problems. Or authors of new code using those > _default helpers from the start. Fair point. I didn't think it was that bad, in fact there were some I was far less sure about, say > + request->page = nla_get_u8_default(info->attrs[NL802154_ATTR_PAGE], > + wpan_phy->current_page); which really depends on the types of 'page' and 'current_page'... I think for most cases it's probably still worth doing, and I wouldn't be _too_ concerned about the type issues here, most places are just using zero or a small constant as the default anyway. Even nla_get_XX_or_zero() would be a win, but that seemed too special ... johannes
