On Mon, Oct 13, 2014 at 4:18 PM, Jonathan Dowland <j...@debian.org> wrote: > On Sun, Oct 12, 2014 at 02:48:55PM -0400, Steve Litt wrote: >> On Sun, 12 Oct 2014 19:02:08 +0100 Martin Read <zen75...@zen.co.uk> wrote: >> > On 12/10/14 18:13, John Hasler wrote: >> > > You have no problem with an 1800 line function? > ... >> > I have a problem with 1800 line functions in general; > ... >> I have no problem with an 1800 line function. > ... > > *What* 1800 line function? The commit URI that was shared was an 1894-line > *file* with a large function definition starting at line 638 and ending at > 1890. That's a 1252-line function.
mmm? 1800 vs. 1252 ? 30 years ago, when we still read printouts, 60 lines was considered the ideal max because that's what would fit on a page. Nowadays, we use a screen, but 60 lines is hard on the eyes (9 pt or so), so 40 lines is a good screen-full. But it turns out, with being about to scroll quickly, that 60 lines is still not hard to reach. Moreover, 60 lines seems to be a pretty good average for what an experienced coder can keep in his head. 1800/60 vs. 1252/60 ? 30 times the ideal max vs. 21 times? (Ignoring, for the sake of your argument, those macros.) Well, maybe we can look at things from the perspective of new functionality. New functionality sometimes breaks rules just because you need to get things in there and going before you can start figuring out where and how to cut things. Okay, that repository only goes back to April 2012: http://cgit.freedesktop.org/systemd/systemd/log/src/core?ofs=1350 at the time of this post. (Give it a month or two, and that link won't go all the way back anymore.) The function in question at that point began at line 545 and ended at line 1540. http://cgit.freedesktop.org/systemd/systemd/tree/src/core/dbus-manager.c?id=b30e2f4c18ad81b04e4314fd191a5d458553773c#n545 That's 996 lines, including the closing brace. Plus-minus one, it's not going to change much. 16.67 times the ideal max, and, for more than a year, it just got bigger until some time after a year ago August. We might assume that non-project people critiquing his code lit a fire under him. > Not only that but you're looking at a commit dating from August last year. The > function doesn't even exist any more in current systemd[1]. There are no > functions of even a 100 lines length in that file now. > > [1] http://cgit.freedesktop.org/systemd/systemd/tree/src/core/dbus-manager.c A quick scan shows a few over the ideal, but the ideal really is an ideal target. So it would actually be reasonable now, in terms of length. If it were not pid 1 code. At least those macros seem to have been replaced with something less fragile. >> What I *DO* have a problem with is the guy's welding pam onto his new init, >> and welding other critical and former separate OS functionalities onto his >> "toolset", preventing (either technically or by them being removed from the >> packages) former modules from being used. > > Which guy is that? The commit that the URI referenced was written by Lennart > Poettering, so I guess you mean him; but that commit didn't touch the file > that > was being complained about. Maybe you mean one of the other 17 people who have > contributed to that file? You do understand that Steve is simply refusing to keep focused on one file? (I don't blame him. That one file is not the sum and end of the problems.) >> If I were to maintain his code, before reducing the 1800 line function, I'd >> do something about the function with 20 arguments, with each argument >> including a function call. I'd replace all of that with a struct pointer. > > I'd start with *reading the code* if I were you; something you guys clearly > aren't doing. How did he know about that 20 parameter function? And don't forget, the file in question was in the source, substantially as it was, for more than a year. How much more, I'll have to find a repository that goes farther back to find out, but I'm not interested. You want to look for it for me? > But if you get past that you'll be pleased to discover that such clean ups and > refactors are happening quite often. Now, at any rate. > See e.g. > df2d202e6ed4001a21c6512c244acad5d4706c87 ("bus: let's simplify things by > getting rid of unnecessary bus parameters"). I'll leave you to guess the > author > of that one. Well, I've done a little mousing around in the repository (current, as well as historical) and it looks like that particular file is part of the pid 1 code. Correct me if I'm wrong. Even conceptually, pid 1 code should not be managing dbus. Too much can go wrong, too many opportunities to get pid 1 chasing it's tail trying to parse an error state. And the code in that file, much improved as it is in current, looks like code that can get into exactly that kind of trouble. I'd have to dig way down deep to be positive, but I'm still extremely unimpressed. Get pid 1 down to 100 lines of C, no loops, no functions called, then I'll be impressed. Heh. No, that's talking about getting Linux ready to compete seriously in embedded, and the kernel needs a bit of work before we start talking about that. (Not that you can't use it now, sans systemd, in some of the less demanding embedded applications. There are some embedded applications it will work for, with reasonable pid 1 processes.) Setting aside initialization code, pid 1 should target less than 1000 lines of C in the main loop. (If we were to use dash or other streamlined shells, we might set a target of 100 lines of code.) Loops and subroutines should be carefully metered for maximum execution paths, and proven to be deterministic, with a maximum execution path of less than 500 lines of C. (I think this target would be significantly better than sysv-init, but you have to set a really good target to get things into a reasonable range.) It should not interface directly with dbus, cgroups, udev, or pamd. It should delegate interfacing to such things to non-pid 1 daemons, and restrict itself strictly to keeping the daemons themselves working together. There are other goals I should mention, but I don't imagine Lennart Poettering is even the slightest interested in what we have to say here, so I won't bother. No matter how much better it is now than last year, the code is still just plain wrong. -- Joel Rees Be careful when you see conspiracy. Look first in your own heart, and ask yourself if you are not your own worst enemy. Arm yourself with knowledge of yourself. -- To UNSUBSCRIBE, email to debian-user-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: https://lists.debian.org/caar43io+cd_fc42ytmch7mtqjm+cocxfqh+n0wvthno8cvx...@mail.gmail.com
