Page MenuHomeFreeBSD

linuxkpi: Don't clobber result on failure
ClosedPublic

Authored by des on Wed, Dec 31, 6:01 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Jan 12, 7:14 AM
Unknown Object (File)
Sat, Jan 10, 4:21 PM
Unknown Object (File)
Sat, Jan 10, 3:29 PM
Unknown Object (File)
Sat, Jan 10, 4:52 AM
Unknown Object (File)
Thu, Jan 8, 10:21 PM
Unknown Object (File)
Wed, Jan 7, 6:30 PM
Unknown Object (File)
Mon, Jan 5, 9:59 AM
Unknown Object (File)
Sat, Jan 3, 11:43 AM

Details

Summary

In kstrto*(), don't assign to *res until we know the conversion is
successful, and address issues that may result in warnings if code
that uses <linux/kstrtox.h> is compiled at high warning levels.

MFC after: 1 week
Sponsored by: Klara, Inc.
Sponsored by: NetApp, Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

des requested review of this revision.Wed, Dec 31, 6:01 PM
This revision is now accepted and ready to land.Wed, Dec 31, 6:06 PM

I've verified that graphics/drm-66-kmod builds cleanly with these changes.

I think I would also always put an (at least) MPASS in there that res is not NULL.
Funny enough kstrtobool() has the check and fails gracefully if *res is NULL.

sys/compat/linuxkpi/common/include/linux/kstrtox.h
82

This may also take a leading + but no minus according to the documentation (unrelated to this fix). Just in case someone wants to do a follow-up.

sys/compat/linuxkpi/common/include/linux/kstrtox.h
82

Forgive me. kstrtoul*l* does, not the one with just one 'l'. The double-ll is further down and would need the check before calling into the other one.

246

This one but not kstrtou64() may also take a leading '+'. (irrelevant to this review; sorry; see above)

sys/compat/linuxkpi/common/include/linux/kstrtox.h
82

I think perhaps someone should write a test suite for the entire family and run it on Linux, and then we can adjust our implementations to match.

In D54440#1244012, @bz wrote:

I think I would also always put an (at least) MPASS in there that res is not NULL.

I'm not sure there's much value in that. A null res is a guaranteed and trivially debugged panic anyway. An assertion makes no difference unless you put it at the top so it triggers even when the conversion would have failed.

Funny enough kstrtobool() has the check and fails gracefully if *res is NULL.

I'd argue that this is a mistake since it converts a guaranteed panic into something that's much harder to debug.

This revision was automatically updated to reflect the committed changes.