Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

`three = 1` in the Linux sourcecode (2014)

github.com

Post reply on HN
Print view View original

11–20 of 105 posts

Re: `three = 1` in the Linux sourcecode (2014)

#11

Post by ronyeh »

hinoki wrote: post #3
post-factum wrote: post #2

705 /* 706 * Iterate through the groups which hold BACKUP superblock/GDT copies in an 707 * ext4 filesystem. The counters should be initialized to 1, 5, and 7 before 708 * calling this for the first time. In a sparse filesystem it will be the 709 * sequence of powers of 3, 5, and 7: 1, 3, 5, 7, 9, 25, 27, 49, 81, ... 710 * For a non-sparse filesystem it will be every group: 1, 2, 3, 4, ... 711 */ 712 static unsigned…

Yeah, maybe calling them pow3, pow5, pow7 would be a bit clearer? But a 10s search for a comment cleared it up, so I don’t really see the problem...

I see a problem:

https://github.com/torvalds/linux/blob/master/fs/ext4/resize...

Upon a quick code review, these lines look buggy:

    unsigned three = 1;
    unsigned five = 5;
    unsigned seven = 7;
Without digging deeper, the reader of this code thinks "The first line surely must be a bug, right???"

Re: `three = 1` in the Linux sourcecode (2014)

#12

Post by scotty79 »

ronyeh wrote: post #11
hinoki wrote: post #3

Earlier quoted context omitted.

Yeah, maybe calling them pow3, pow5, pow7 would be a bit clearer? But a 10s search for a comment cleared it up, so I don’t really see the problem...

I see a problem: https://github.com/torvalds/linux/blob/master/fs/ext4/resize... Upon a quick code review, these lines look buggy: unsigned three = 1; unsigned five = 5; unsigned seven = 7; Without digging deeper, the reader of this code thinks "The first line surely must be a bug, right???"

Not really. Nobody sane would think other sane developer with any expeirience would call a simple variable according to what it initially contains. You'd expect either name related to what is the meaning of the contents, or meaningless name.

Re: `three = 1` in the Linux sourcecode (2014)

#13

Post by oji0hub »

scotty79 wrote: post #12
ronyeh wrote: post #11

Earlier quoted context omitted.

I see a problem: https://github.com/torvalds/linux/blob/master/fs/ext4/resize... Upon a quick code review, these lines look buggy: unsigned three = 1; unsigned five = 5; unsigned seven = 7; Without digging deeper, the reader of this code thinks "The first line surely must be a bug, right???"

Not really. Nobody sane would think other sane developer with any expeirience would call a simple variable according to what it initially contains. You'd expect either name related to what is the meaning of the contents, or meaningless name.

People make typos and simple mistakes all the time, regardless of experience.

Re: `three = 1` in the Linux sourcecode (2014)

#14

Post by Guthur »

oji0hub wrote: post #13
scotty79 wrote: post #12

Earlier quoted context omitted.

Not really. Nobody sane would think other sane developer with any expeirience would call a simple variable according to what it initially contains. You'd expect either name related to what is the meaning of the contents, or meaningless name.

People make typos and simple mistakes all the time, regardless of experience.

+1 seen it so many times, often times from some cut and paste heavy refactor, one is more or less going on autopilot just moving substrings around and then bam. Review process hopefully catches it, when it is a bug, and potentially when the semantics are unclear...

Re: `three = 1` in the Linux sourcecode (2014)

#15

Post by derfitt »

ronyeh wrote: post #11
hinoki wrote: post #3

Earlier quoted context omitted.

Yeah, maybe calling them pow3, pow5, pow7 would be a bit clearer? But a 10s search for a comment cleared it up, so I don’t really see the problem...

I see a problem: https://github.com/torvalds/linux/blob/master/fs/ext4/resize... Upon a quick code review, these lines look buggy: unsigned three = 1; unsigned five = 5; unsigned seven = 7; Without digging deeper, the reader of this code thinks "The first line surely must be a bug, right???"

'Without digging deeper' isn't really what matters here. A better question would be 'after some very cursory further digging, are we worried?' The first thing I ask myself when I see something that looks as wrong as this does is "Is it possible for this piece of code to be completely broken, and for no one to have noticed yet?"

This is really a question about two processes - the one where people look at, read, edit and discuss the code, and the extent to which this is discoverable by them. And the one where people run the code and expect certain things to work and would notice if they didn't work, or didn't work correctly.

This fails both of those checks. This file has been edited 8 times in the last year, by 5 different people. It's not so long that several of them wouldn't have read through the whole thing. This bug, if it was a bug, is unlikely to have persisted for 6 years without being noticed. It's not obfuscated or deeply technical in any way.

Secondly, this is code which is fairly critical to millions of systems. Ext4s is in literally hundreds of millions of devices. It's used in data centers in contexts where people are likely to follow up on any unusual behavior by their filesystem. The code in question is likely to be very well exercised in tests. It's likely to be extremely well exercised by people doing research stress testing/comparison testing of filesystem code. It is undoubtedly very well exercised by real-world deployments.

The bar to assume that you know something the person who wrote the code didn't, as opposed to the other way around, should be very high here. Even taking all the benefit of the doubt (which you should always do, never assume that stuff 'probably works' without evidence) it's not hard to dismiss this.

Re: `three = 1` in the Linux sourcecode (2014)

#16

Post by ant6n »

GuB-42 wrote: post #6

Another fun one is ONE = 256 (or another power of two). Intended for fixed point arithmetic where 1 actually means 1/256.

This is nicer example, since the posted one should really have different variable names (e.g. pow3). On the other hand, if this declaration of ONE is global (not contained within some context like a type), then perhaps it should be something like ONE_F8.

Re: `three = 1` in the Linux sourcecode (2014)

#17

Post by croes »

hinoki wrote: post #3
post-factum wrote: post #2

705 /* 706 * Iterate through the groups which hold BACKUP superblock/GDT copies in an 707 * ext4 filesystem. The counters should be initialized to 1, 5, and 7 before 708 * calling this for the first time. In a sparse filesystem it will be the 709 * sequence of powers of 3, 5, and 7: 1, 3, 5, 7, 9, 25, 27, 49, 81, ... 710 * For a non-sparse filesystem it will be every group: 1, 2, 3, 4, ... 711 */ 712 static unsigned…

Yeah, maybe calling them pow3, pow5, pow7 would be a bit clearer? But a 10s search for a comment cleared it up, so I don’t really see the problem...

Pow3, pow5, pow7 still sounds wrong without explanation why pow3 starts at 3^0 but pow5 and pow7 start at 5^1 and 7^1

Re: `three = 1` in the Linux sourcecode (2014)

#19

Post by nxpnsv »

ronyeh wrote: post #11
hinoki wrote: post #3

Earlier quoted context omitted.

Yeah, maybe calling them pow3, pow5, pow7 would be a bit clearer? But a 10s search for a comment cleared it up, so I don’t really see the problem...

I see a problem: https://github.com/torvalds/linux/blob/master/fs/ext4/resize... Upon a quick code review, these lines look buggy: unsigned three = 1; unsigned five = 5; unsigned seven = 7; Without digging deeper, the reader of this code thinks "The first line surely must be a bug, right???"

Lesson: dig deeper.

Re: `three = 1` in the Linux sourcecode (2014)

#20

Post by mnw21cam »

derfitt wrote: post #15
ronyeh wrote: post #11

Earlier quoted context omitted.

I see a problem: https://github.com/torvalds/linux/blob/master/fs/ext4/resize... Upon a quick code review, these lines look buggy: unsigned three = 1; unsigned five = 5; unsigned seven = 7; Without digging deeper, the reader of this code thinks "The first line surely must be a bug, right???"

'Without digging deeper' isn't really what matters here. A better question would be 'after some very cursory further digging, are we worried?' The first thing I ask myself when I see something that looks as wrong as this does is "Is it possible for this piece of code to be completely broken, and for no one to have noticed yet?" This is really a question about two processes - the one where people look at, read, edit a…

The problem with this argument is that this particular snippet of code doesn't get run very often. It's the bit that works out where the superblock (and all its backups) are stored. Now, normally, only the one copy of the superblock is ever read - the backups are only read if the first copy of it is corrupted in some way. So almost all the time, the results of this code are not used. So if it were broken, then yes it could possibly go unnoticed for 6 years.
Post reply on HN