Re: [PATCH 1115/1285] Replace numeric parameter like 0444 with macro | Hacker News Reader

Re: [PATCH 1115/1285] Replace numeric parameter like 0444 with macro

lkml.org

57 pointssslalready44 comments

Linus' reply: "I detest these patches."

https://lkml.org/lkml/2016/8/2/1589

> "I don't want to see _any_ of these patches being applied."

>"And part of it is exactly that there are nineteen billion of these "random letter combinations" "

Another option to understand the magnitude of this fuck up is to have a look at today's chronological email list: https://lkml.org/lkml/2016/8/2/

My favourite thing about it is that there was no header email (usually [PATCH 0/n] is just text with an overview of the following patches. Then any top-level discussion (I.e. "NAK: octal is more readable") takes place in replies to that email.

Instead here there are 1285 separate threads of discussion with dozens of people all saying the same thing with no easy way to discover it's already been said (and at least one maintainer has replied to every single one he's CC'd on). Beautiful carnage.

It's an interesting question as to which is more readable:

    -module_param(max_sets, int, 0600);
    +module_param(max_sets, int, S_IRUSR | S_IWUSR);

However, I'm missing context. Did someone at Intel really spam 1,285 patches without any prior discussion?

Seems like it. I would hope Intel wouldn't hire someone that would think this is OK ... OTOH see Uber's writeup about how developers can't be expected to think about transactions.

Maybe there's some perverse incentive at Intel? Like performance review that considers how many patches you've sent? Or maybe its a broken tool to help submit patches?

I'd be inclined to think it was a commit per smaller change set and each commit got turned into a separate patch.

It looks like it got turned into one commit per file. Which is pretty atrocious.

Isn't "one commit per independent change" common advice in git tutorials? Each file can be changed independently of any other, so taken literally, exactly how you're supposed to do it.

That's wildly incorrect. "One commit per logical change" would mean that every single file modified in this way should have been in a single commit. By your logic, you could never have two files changes in the same commit.

> you could never have two files changes in the same commit.

We're going OT here, but i don't think that applies to C, where a "logical change" to a function definition may be split over two files (.c/.h).

In the kernel, the proper practice for sweeping changes like this is one commit per subsystem.

There are some mixed opinions in the maintainers' replies. Steve Rostadt is NACKing all of his patches (amusingly also spamming everyone in the process) since he likes the octal and others are making the same argument. Other again are suggesting more use of S_IRUGO since the author seems to have broken up a large number of octal macros into naive combinations of the S_* macros without much thought for semantic intention. A hardware maintainer encouraged the use of some ATTR_RW macros which indicates that the author didn't necessarily recognise the preferred tropes for various subsystems.

The backdoor argument (mentioned in an earlier email in the OP thread) is pretty reasonable to raise. The maintainers seems to be fairly on top of things though, acking and nacking as patches come in but this will take a while to process. I suspect very much that these commits will have to be rewritten to include subsystem labels and more accurate summaries by the maintainers if they want to include them. The patch set as an aggregate whole is garbage.

I can understand that for someone has no background or long term use of chmod(1) would need some effort to decipher what 0644 actually means. However, for people who are well versed in this aspect, the numerical code is much easier to parse.

This reminds me of constantly looking up j dict [0] before I could remember what each number means. J's foreign conjunction is certainly an extreme here, but it teaches me that what is easy to parse really depends on one's proficiency in the language.

[0] http://www.jsoftware.com/help/dictionary/dx001.htm

I've been using the octal for many years, but never with any real frequency so I always have to think about it a bit more than I'd like to admit. Having said that, looking what those patches are proposing... the octal is far superior even for an interloper like myself. I know how to think about the octal permissions to get to what is happening without digging into documentation... I can't say that with the macros.

Also, just spamming like that is insane.

I've been using Unix for years and years on a variety of systems, including various sysadminy roles, and I still cannot keep the octal permission numbers in my head. My current job involves using one tool which only supports the octal and I always go to an online site which calculates them for me because otherwise I will just get them wrong.

I'd vastly prefer the symbolic forms (and am slightly surprised that the kernel source has raw octal in it at all). Although I wouldn't have tried to implement the change as a thousand separate patches...

chmod(1) man page would either contain the meaning of each number or a reference to where the definitions are on any decent Unix system.

The symbolic form is frankly unreadable and the kernel people don't need them because they probably already dream of octal permissions when asleep.

For an alternative symbolic approach, here is how it is done in Lisp, thanks to SETF (and the OSICAT library).

    (file-permissions #P"/tmp/file")
    NIL

    (setf (file-permissions #P"/tmp/file")
          '(:user-read :group-read :other-read))

    ... or

    (nix:chmod #P"/tmp/file" #o444)

By the magic of SETF, you can now use all other macros which modify a place:

    (push :user-write (file-permissions #P"/tmp/file"))

And thus:

    (file-permissions #P"/tmp/file")
    => (:USER-READ :USER-WRITE :GROUP-READ :OTHER-READ)

I'm the exact opposite - I can instantly understand what 600, 777, 664 and so on mean with barely a thought, but having used Linux for the majority of my life I still struggle to remember "chmod u+w" and whatever.

Those are different things though. "u+w" doesn't require me to know what the current permissions are, but only that I want to let the owner write.

I think what trips me up about them is that they are effectively counted backwards. RWX is 421 rather than 124.

The octal representation is such a deep-grained part of Unix on all levels (both C and shell) that replacing it is futile. These weird 0-prefixed octal constants will still be in common use a hundred years from now, for better and worse.

Also, it's not easy to argue that "S_IRUSR | S_IRGRP | S_IROTH" is any kind of readability improvement...

Linus is right because these are actually well known and easy to parse.

However, if instead of S_IRUSR|S_IWUSR (which I'd have to lookup up to parse correctly), it were possible to write "a-rwx"|"u+rw", it would be even easier to parse, without the need to memorize common octal numbers. In C, this could be achieved with the preprocessor, I believe, if you create a sufficiently smart CPP macro, especially since Linux is no stranger to heavy preprocessor use.

Yes, you can certainly achieve that with setmode(3) and getmode(3).

Not in the kernel, you can't :)

Sounds like someone ran a script without thinking too much.

Shameful to see @intel.com in the signed-off-by on this spam train

baolex.ni AT intel is also who fired off this patch bomb.

I noticed that HP (now HPE) began locking access to server UEFI updates soon after http://lkml.org/lkml/2013/11/11/653 was posted.

Why is it shameful? Someone considered this an improvement, went through the process of creating >1000 patches and submitted them for review. That the patches were rejected categorically and someone presumably spent consideralbe development time on them is also not bad, because the likelihood of a good response to a question "what if I replaced .. with .." is low and seeing the huge patchset also drives home the amount of magic numbers one might replace.

It's just how software is developed where code is reviewed before inclusion and not committed-by-default and backed out later. And there's no Intel pre-mailing patch review group that has to vet kernel patches before a dev is allowed to send them out, ignoring NDA'ed things, of course.

There is no ill will here, and Intel has more unseasoned Linux developers now, so it's normal for seemingly naive patches to appear.

We need more developers who try, fail, try again, and that requires less shaming and more encouragement.

Blasting over 1200 emails to a mailing list with no heads-up appears quite tone-deaf, to put it mildly even if you presume good intentions.

Edit: The impact could be shown in a single email with a diffstat, for example. Complaining about a lack of feedback for such a thing sounds more like complaining about failing to draw others in to a bikeshedding competition.

Fair point, though LKML is very high volume, so not as much of a big deal as one might think.

> The impact could be shown in a single email with a diffstat, for example.

True, and I agree, but not the way stuff is usually done on LKML, thus probably not considered.

In this special case of a huge number of patches it might have been a good idea to present an RFC diffstat first, yes. Actually, I wish github had a .diffstat extension for pull requests, because often that's the first thing I care about before I know if I want to delve into a random big patch.

Blasting over 1200 emails to a mailing list with no heads-up appears quite tone-deaf

Compared to the bits and pieces of LKML I occasionally see, blasting 1200 emails is downright polite.

Or a initial email going "heads up, big patch set coming through!", with the patch set as replies to that one.

That's usually what happens with a git formatted patchset that has you draft a patch-0 (aka introduction of the series) email.

>That the patches were rejected categorically and someone presumably spent consideralbe development time on them

These were obviously generated programattically. I refuse to believe otherwise.

The macros are for when you're reading the value, not setting it:

    if(m & S_IRUSR) { ... }

That doesn't make a lot of sense. If you know the octal numbers, you don't forget them when reading the value.

These numbers are almost hardwired in my brain and I am not even a kernel developer. I think most kernel developers should not need the help of these magic macros.

I'd be curious to see a graph of the load average on vger.kernel.org today. I can imagine a huge spike lasting several hours.

Why did each of those need to be a simple patch? Couldn't they be just one big patch, which would have been a little easier to read?

(kernel muggle here, mind)

They didn't. Many maintainers will NAK purely because of that.

Reply on news.ycombinator.com