On Tue, Jun 14, 2011 at 5:15 AM, Denys Vlasenko
<vda.linux@googlemail.com> wrote:
> I've got a patch for my project to fix parsing of kernel version which
> has only two numbers. Basically,
> - scanf(ver, "%u.%u.%u", &a, &b, &c)
> + sscanf(ver, "%u.%u", &a, &b)
Please just fix it.
The projects that care about kernel version are buggy in so many ways
that it's not funny.
It's not just the "two versus three digits" bug either. The bigger
bug is usually that the tests are done the wrong way entirely, namely
to fail when you cannot parse the version. And that's just f%!ing
stupid!
If you cannot parse the version, then rather than fail, a project
should have gone "uh, it's some future version that I don't recognize
or some other OS entirely, so I'll just do whatever the most modern
thing is". Having an "assert()" or returning an error is just the mark
of incompetence.
So please just fix things. Preferably by removing the version check
entirely, but if you really feel that you cannot do that, then AT
LEAST realize that the reason you're looking at the version is to
support old versions, not new ones, and make the logic work that way
(so that next time around, when we change the version numbers to be
sanscrit characters and you fail parsing them again, you don't
actually fail, you just go "oh, this is modern" and go on with your
life).
He's got a really good point about version number checking being for supporting old versions, not new ones.
I suppose it's probably that quotes are selected for their brilliance, but bits of logic like Linus' in that post always awe me. They're so simple and right when you hear them, but you didn't think of them yourself.
Yep, he's pointing out that checking a version string doesn't actually check that the version you are running actually supports what you want.
One reason why in my code I check how something behaves and rarely for a version number. Test that a feature exists or not and base your behavior on that instead.
This is why autoconf annoys me. Sometimes it will actually check functionality, but most people just write rules to compare against version numbers. And of course they get it wrong and think that 1.8.12rc2 is somehow less recent than 1.8.9. Don't tell me it won't work, try it and show me it won't work.
The code in question is for parsing command-line arguments, and its goal is to determine if the user was specifying an (optional) kernel version. When the goal is to guess what a user is intending, you're inherently dependent on _name_ of the version, not its features. And, contrary to what Linus says, you care just as much about future versions as old ones.
So Linus's criticism is almost entirely wrong, for the actual use-case of the author of this request. There are other alternate criticisms, perhaps, and of course Linus's criticism does apply to most people who write "check this version" code.
This is perfectly analogous to websites checking for specific browser versions.
So many websites exist where you'll browse with Chrome or Firefox and you'll get a big red "Looks like you're using an old version of Internet Explorer, please upgrade to IE6 or newer" just because they're checking your user-agent header rather than availability of specific features. This is what Linus is trying to avoid in the Linux ecosystem—a noble cause.
9 comments
[ 2.8 ms ] story [ 30.5 ms ] thread> I've got a patch for my project to fix parsing of kernel version which
> has only two numbers. Basically,
> - scanf(ver, "%u.%u.%u", &a, &b, &c)
> + sscanf(ver, "%u.%u", &a, &b)
Please just fix it.
The projects that care about kernel version are buggy in so many ways that it's not funny.
It's not just the "two versus three digits" bug either. The bigger bug is usually that the tests are done the wrong way entirely, namely to fail when you cannot parse the version. And that's just f%!ing stupid!
If you cannot parse the version, then rather than fail, a project should have gone "uh, it's some future version that I don't recognize or some other OS entirely, so I'll just do whatever the most modern thing is". Having an "assert()" or returning an error is just the mark of incompetence.
So please just fix things. Preferably by removing the version check entirely, but if you really feel that you cannot do that, then AT LEAST realize that the reason you're looking at the version is to support old versions, not new ones, and make the logic work that way (so that next time around, when we change the version numbers to be sanscrit characters and you fail parsing them again, you don't actually fail, you just go "oh, this is modern" and go on with your life).
In other words: don't be stupid.
Ok?
I suppose it's probably that quotes are selected for their brilliance, but bits of logic like Linus' in that post always awe me. They're so simple and right when you hear them, but you didn't think of them yourself.
One reason why in my code I check how something behaves and rarely for a version number. Test that a feature exists or not and base your behavior on that instead.
https://lkml.org/lkml/2011/6/14/192
And/or a clarification someone else gave to Linus:
https://lkml.org/lkml/2011/6/15/38
The code in question is for parsing command-line arguments, and its goal is to determine if the user was specifying an (optional) kernel version. When the goal is to guess what a user is intending, you're inherently dependent on _name_ of the version, not its features. And, contrary to what Linus says, you care just as much about future versions as old ones.
So Linus's criticism is almost entirely wrong, for the actual use-case of the author of this request. There are other alternate criticisms, perhaps, and of course Linus's criticism does apply to most people who write "check this version" code.
So many websites exist where you'll browse with Chrome or Firefox and you'll get a big red "Looks like you're using an old version of Internet Explorer, please upgrade to IE6 or newer" just because they're checking your user-agent header rather than availability of specific features. This is what Linus is trying to avoid in the Linux ecosystem—a noble cause.