8 ms·

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

57 points by sslalready 10y ago

SloopJon 10y ago
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?
- MichaelGG 10y ago
  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?
  - emeraldd 10y ago
    I'd be inclined to think it was a commit per smaller change set and each commit got turned into a separate patch.
    - eridius 10y ago
      It looks like it got turned into one commit per file. Which is pretty atrocious.
      - tedunangst 10y ago
        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.
        eridius 10y ago
        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.
        mappu 10y ago
        > 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).
        bjackman 10y ago
        In the kernel, the proper practice for sweeping changes like this is one commit per subsystem.
- ecma 10y ago
  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.
abstractbeliefs 10y ago
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/ https://lkml.org/lkml/2016/8/2/
- bjackman 10y ago
  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.
Artlav 10y ago
Sounds like someone ran a script without thinking too much.
deleted 10y ago
[deleted]
DblPlusUngood 10y ago
Linus' reply: "I detest these patches." https://lkml.org/lkml/2016/8/2/1589 https://lkml.org/lkml/2016/8/2/1589
- jlgaddis 10y ago
  > "I don't want to see _any_ of these patches being applied."
  - totony 10y ago
    >"And part of it is exactly that there are nineteen billion of these "random letter combinations" "
0x0 10y ago
Shameful to see @intel.com in the signed-off-by on this spam train
- jlgaddis 10y ago
  baolex.ni AT intel is also who fired off this patch bomb.
- yuhong 10y ago
  I noticed that HP (now HPE) began locking access to server UEFI updates soon after http://lkml.org/lkml/2013/11/11/653 http://lkml.org/lkml/2013/11/11/653 was posted.
- cm3 10y ago
  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.
  - 0x0 10y ago
    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.
jlgaddis 10y ago
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.
cm3 10y ago
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.
- jxy 10y ago
  Yes, you can certainly achieve that with setmode(3) and getmode(3).
  - ecma 10y ago
    Not in the kernel, you can't :)
pavlov 10y ago
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...
raldi 10y ago
The macros are for when you're reading the value, not setting it: if(m & S_IRUSR) { ... }
- tedunangst 10y ago
  That doesn't make a lot of sense. If you know the octal numbers, you don't forget them when reading the value.
jxy 10y ago
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 http://www.jsoftware.com/help/dictionary/dx001.htm
- sbuttgereit 10y ago
  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.
- david-given 10y ago
  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...
  - gkya 10y ago
    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.
  - junke 10y ago
    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)
chj 10y ago
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.
chj 10y ago
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.
YeGoblynQueenne 10y ago
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)
- ajdlinux 10y ago
  They didn't. Many maintainers will NAK purely because of that.

57 points by sslalready 10y ago

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?

MichaelGG 10y ago
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?
- emeraldd 10y ago
  I'd be inclined to think it was a commit per smaller change set and each commit got turned into a separate patch.
  - eridius 10y ago
    It looks like it got turned into one commit per file. Which is pretty atrocious.
    - tedunangst 10y ago
      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.
      - eridius 10y ago
        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.
        mappu 10y ago
        > 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).
      - bjackman 10y ago
        In the kernel, the proper practice for sweeping changes like this is one commit per subsystem.
ecma 10y ago
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.

abstractbeliefs 10y ago

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/ https://lkml.org/lkml/2016/8/2/

bjackman 10y ago
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.

Artlav 10y ago

Sounds like someone ran a script without thinking too much.

deleted 10y ago

[deleted]

DblPlusUngood 10y ago

Linus' reply: "I detest these patches." https://lkml.org/lkml/2016/8/2/1589 https://lkml.org/lkml/2016/8/2/1589

jlgaddis 10y ago
> "I don't want to see _any_ of these patches being applied."
- totony 10y ago
  >"And part of it is exactly that there are nineteen billion of these "random letter combinations" "

0x0 10y ago

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

jlgaddis 10y ago
baolex.ni AT intel is also who fired off this patch bomb.
yuhong 10y ago
I noticed that HP (now HPE) began locking access to server UEFI updates soon after http://lkml.org/lkml/2013/11/11/653 http://lkml.org/lkml/2013/11/11/653 was posted.
cm3 10y ago
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.
- 0x0 10y ago
  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.

jlgaddis 10y ago

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.

cm3 10y ago

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.

jxy 10y ago
Yes, you can certainly achieve that with setmode(3) and getmode(3).
- ecma 10y ago
  Not in the kernel, you can't :)

pavlov 10y ago

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...

raldi 10y ago

The macros are for when you're reading the value, not setting it: if(m & S_IRUSR) { ... }

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

jxy 10y ago

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 http://www.jsoftware.com/help/dictionary/dx001.htm

sbuttgereit 10y ago
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.
david-given 10y ago
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...
- gkya 10y ago
  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.
- junke 10y ago
  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)

chj 10y ago

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.

chj 10y ago

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.

YeGoblynQueenne 10y ago

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)

ajdlinux 10y ago
They didn't. Many maintainers will NAK purely because of that.