Hacker News Reader

Comment by barrkel | Hacker News Reader

How familiar are you with largish C codebases?

At this level, you're dealing with an API at the calling end (you should be fairly intimately familiar with the semantics without much reference to the docs if you're working on the implementation) and the functions being calling are almost readable as English.

Code should strive to be written such that it doesn't need comments unless it's being clever - and it should avoid being clever if possible.

Also, I'm not sure where you saw a "wall of code". I don't see any walls of code, not in the commit diff, nor in the smaller diff in the email. A wall of code, for me, would have to be long (say, 70+ lines - but it depends on the language) and dense (e.g. boolean expressions complex enough to need parentheses to clarify precedence).

View on HN

> How familiar are you with largish C codebases?

Well I don't know, let's see some code for postgresql, a quite large C codebase. I took this file at random:

https://github.com/postgres/postgres/blob/master/src/pl/plpg...

So assuming "what you know" means the same as "what is the best" is probably not a good idea.

That's an attitude of "stay the fuck out of my code - you're too stupid to understand it without comments". I've seen it too often in "large code bases"

"you should be fairly intimately familiar with the semantics" - do you see a problem bootstrapping that on an obscure code base?

> …do you see a problem bootstrapping that on an obscure code base?

No, I don't. It just requires that you can read C and understand it. Any large codebase (in whatever language) is going to require you to understand certain semantics and idioms, and it doesn't make any sense to document those idioms every single place you use them. Big codebases require context and a comment in some random function isn't going to give you enough context unless it was a tome.

Also, did you see the 2 patches linked in the bug report? They significantly changed the code. C is just like that. It's an environment ripe for comment bitrot. And the only thing worse than no comments is incorrect comments.

In the end, the code is what matters. If you can read it, you can understand it.

Yes, I've seen, read and kind of felt what the patch does. If you claim you _understand_ the code then I call you on this.

The only thing worse about something is the preconceived idea that there is indeed only a _single_ thing worse - all the rest being better or equal.

No, worse than missing comments is also _bad code_ which was the case here. And bad attitudes like "my code is prone to bit rot so I won't comment it" or "my code is self-explanatory so I won't comment it" or worse "my code is so special and super optimized and smart that I will not comment it so only super special guys will be able to modify it". And also "I'm a kernel maintainer so I don't have to comment anything"

Excuse me, based on which experience or do you claim that it was bad code? Based on which experience do you claim that there should be more comments? It looks to me that you don't have anything to support what you say, and I'd appreciate you showing me the opposite.

Have you actually ever programmed anything for Linux kernel? What have you programmed that would be relevant and give the weight to your (for me dubious) claims?

I don't claim anything. The report claims "This appears to be a serious bug". If it wasn't, then they would have not suggested a patch and we would have not have this conversation.

Now, I don't have to show you anything. What are you talking about?

Do you dismiss me because I extrapolate developer attitudes from their obtuse/obscure code (dubious/discuss) or because I can't keep a stack of callbacks in my mind to get myself out of the maze (true by the way)?

But what about that function? It has 5 arguments! Do you claim that they need no explanation whatsoever? And don't cheat. Tell me without looking what is "type" suppose to contain?

[1]http://lxr.free-electrons.com/source/net/compat.c#L223

It was bad code because it dereferenced a user mode pointer without going through copy_from_user/copy_to_user. That much is clear.

That's not to say that nobody else has made this mistake or that it won't be made again by otherwise capable programmers. One can talk about this as bad code while still having sympathies for the error. Hate the sin, not the sinner.

The issue I raised is not about this particular piece of code or about it being defective - it could have been perfect for what is worth.

The deeper issue is about code as it is communicated to the next developer. For various reasons (most - me included) developers do not talk to the next developer but to the compiler. And some other guy comes later who doesn't know about copy_to/from_user shit because he is not exposed to the internals.

He might be competent programmer but he lacks exposure. He might also be able to see in a second the dereference but when he is reviewing the code, he keeps a stack of several levels of irrelevant contexts that blurred his view.

He could have avoided all that if only the code was being written with him in mind.

Having done a small amount of work with the Linux kernel I do not consider this to be an informed position. There are a lot of conventions being followed here and they are followed consistently throughout the code base - they become more and more obvious the more code you read. Not knowing about copy_{from,to}_user is a very rookie mistake. I find it hard to believe the author of this code didn't know about it (they even annotated the parameter as a user pointer), my guess is they probably just figured that the functions they were calling already performed this check and didn't think it needed to be inserted twice.

Edit: Re-reading your comment I also wanted to point out that the problem of handling user mode pointers in a kernel is a difficult one, and requires discipline. You can't keep a straight face and say as you do that you can handle it in a way that newbies can come right in and write bug free code without learning these conventions. The only sane way is to create a workable set of conventions, apply them consistently, and do what you can to educate new contributors as they come along. It's not unique to Linux - I used to work at MS and can tell you the NT kernel has the same issues.

Man - don't take my example as something to extrapolate from. See the big picture and where my example comes from. Extrapolate from there.

This particular piece of code is simple and easy to understand and follow. The function is small - you can take a few hours and understand the stack.

The point is, you have to play the compiler game in order to understand. The original coder is not helping you in any way. It doesn't give you any context. Where's the place of this (any) function? Why was it written in the first place? What problem is it trying to solve?

There are some things that the code alone can't tell. Because the code talks to the compiler and the compiler doesn't care about context - people do. And people understand either by taking the hard way (playing the compiler game - having to jump back & forth) or talking to a human. And the code comments are the best that you can get short of talking directly to the developer.

I think you have abstracted yourself too far away from the details here.

I'm all about leaving code maintainable for the next guy, however I think we disagree about what that means. To me, it's largely about writing your code in consistent patterns that make certain classes of bug "pop out" at you (because doing things that way would stand out and look like a break from the convention).

However as a practical concern, if you're writing kernel code you shouldn't assume your future maintainers don't know about the distinction between user and kernel address spaces. And you shouldn't have to be very detailed about every small little compatibility shim you write - if compat_sys_recvfrom does very little else but call sys_recvfrom without much comment or fanfare that is OK by me (keep in mind there's going to be one of these wrappers for almost every syscall).

Indeed, you have to draw a line somewhere. Do you explain (1) 2+2, (2) user/kernel space, (3) what a _compat usually is for or (4) what this particular _compat does?

I for one would draw the line before (4). The original developer drew it after. If I would explain API conventions I would draw it before (3) but crypto _API_ for example has the line after (4). And there are appropriate places for (1) and (2) also.

The point is, you address to some audience. But even if the code is clean, a newcomer will have a hard time getting up to speed if he has no idea about the context. The way I see it, for some parts of the kernel, either the code is too precious to be touched or the maintainers do not have any interest in explaining it to an outsider. It's like they are loosing knowledge by sharing.

Think new comers. How can you make them contribute? Or maybe the barrier to entry is high enough on purpose?

I'm reading Clean Code (again). I dont see that code as being particularly bad. But it definitely wouldnt follow the axioms in that highly regarded book. I realize it isnt java but can someone familiar with the text comment on the number of parameters in that function and the naming of some of those variables?

Relevant quote: "The ideal number of arguments for a function is zero (niladic). Next comes one (monadic), followed closely by two (dyadic). Three arguments (triadic) should be avoided where possible. More than three (polyadic) requires very special justiﬁcation—and then shouldn’t be used anyway."

Zero? So not even pure functional programming is OK, since that tends to pass state as function arguments? We should all just happily poke our global state from our niladic functions?

Haven't read that book, but color me neon skeptical.

Well POSIX requires more than three arguments for some system calls. But around 6 is the limit for most architectures. They are passed in registers for efficiency.

So, the function in question operates on a socket object. In Java that would be implicit as the object on which you invoke the method, but in C it must be an explicit function argument - that accounts for the first `fd` parameter.

The second mmsg parameter points to an array of compat_mmsg objects. In C, the array length must be passed as a separate parameter, so the second and third parameters are really one logical array parameter.

I find the idea that the ideal function takes no arguments to be astonishing. There are only two kinds of functions that take no arguments:

1. Those that return the same result every time.

2. Those that mutate some internal state (or, roughly equivalently, those that inspect internal state being mutated elsewhere).

#1 isn't all that useful in general, although there are obviously cases where it's exactly what you want. For #2, while state is useful at times and a necessary evil in many other cases, it's hardly ideal.

Even a single parameter seems decidedly un-ideal. To do useful work in general, you're typically going to want to take two parameters: something to be modified, and the modification to make. This can be done with state (modify in place) or functionally (return a new object with the modifications applied). Once again, having a single argument seems to imply either something not very useful (basically a getter or some similar derive-a-value function) or something relying on mutable state. You need two arguments to achieve the sort of combinatorial power that makes functions interesting and properly useful.

I wonder if the book, which I assume is talking about Java, is ignoring the implicit zeroth argument which is the object you're invoking the function on. Obviously in C you have to make that parameter explicit, but I've got enough experience of both languages to know that people often don't consider the object to be a parameter to one of its member functions.

So if you're converting that rule back to C, you'd need to add one to each element of the rule. The one parameter rule is saying that when special cases are common, a special function to handle it is a good thing. So writing `i++` rather than `i = i + 1` or `next(node)` rather than `skip(node,1)`.

That would make sense, except for the part where Java doesn't have functions at all, but rather methods. I imagine you could call them functions, but I haven't seen it done, and that sort of terminology sloppiness leads exactly to confusion like this.

Thanks for the responses. The text is definitely referring to Java methods as functions, and not counting the implicit object instance (this) as a parameter. The text also considers multiple parameters of the same type that are treated identically as lists counting as one parameter.

Appreciate the clarification. The advice sounds much more sane that way. It sounds like a terribly confusing way to put it, but maybe it's better in context.

Reply on news.ycombinator.com