11 ms·

I agree that the maintainers work is better. But he could have just done this as a code review on the patch and ask for an update. That happens constantly for

I agree that the maintainers work is better. But he could have just done this as a code review on the patch and ask for an update. That happens constantly for new contributors to a project and it is the proper way to do things - it teaches people and brings them up to speed. If you just redo their work, you are discouraging them from contributing while also not allowing for a teachable moment.

jacquesm 3y ago
Not on a security related list though. That's entirely the wrong expectation.
- varispeed 3y ago
  Security through obscurity? Oh...
- Arnavion 3y ago
  linuxppc-dev is not a security list.
  - jacquesm 3y ago
    It was cc-'d to the security list.
    - deleted 3y ago
      [deleted]
    - Arnavion 3y ago
      On the first patch that Miculas submitted with their gmail address that was not looked at by any of the parties involved in the rest of the story. The subsequent mails to linuxppc-dev where Leroy and Ellerman were involved were not cross-posted AFAICT.
      - jacquesm 3y ago
        Yes, that's correct. But the OP didn't have to CC the security mailing list in the first place and that changed the dynamic considerably. Note that the degree to which this has blown up is disproportional and that besides the fact that Michael Ellerman could have handled this more gracefully the OP did not seek to resolve this out of band, misrepresented the interaction and ignored an olive branch that would have given him exactly what he wanted. The bit where I am in strict disagreement with how Michael Ellerman handled this is actually the reverse of what happened: if the original patch wasn't properly signed off on and the OP was responsive they actually put themselves at risk by re-using the code without proper attribution. This - and that's speculative - probably was because the code identified the essence of the fix but introduced a couple of new problems and because it was so small he may have felt that fixing it and adding a 'Reported-by' tag plus the LKML archive would be sufficient cover for that. But from a strict reading of the LKML guidelines that was the wrong thing to do. And the LKML guidelines themselves should be made clearer with respect to how the various levels of contribution are dealt with because that seems to matter a lot to some people and they need to spell out more clearly that submitting a patch may result in your code ending up in the kernel with LKML as the only visible proof that this is the case. Because the OP may not realize it but that is the essential part: that their submission to LKML ended with a bug in the Linux kernel fixed and there is ample proof to document that and I'm pretty sure that nobody would claim otherwise.
fsdjkflsjfsoij 3y ago
A lot of these maintainers are already overworked and don't have time to tutor every single person that wants to put kernel contributor on their resume. A lot of people have this extremely idealistic view of open source but the reality is that the vast majority of patches you receive will be atrocious and you shouldn't have to tutor every single "potential" contributor. This case is even worse since this issue was posted to a security mailing list which means time to resolution is important.
- ChildOfEru 3y ago
  Who knows how long this would have been unaddressed without Ariel Miculas initial input. Part of the job of a maintainer is to curate and foster input so the project has a robust community in the future. The maintainer in this case did a miserable job of that.
- a_t48 3y ago
  On the other hand, to dig yourself out of the hole of “I’m too busy and there’s nobody around to help”, you have to slow down and do this, sometimes.
  - bhouston 3y ago
    > to dig yourself out of the hole of “I’m too busy and there’s nobody around to help”, you have to slow down and do this Exactly! If you treat new contributors as crap because you are overworked, you will push away potential long-term collaborators who could share the load thus ensuring you will be working alone far into the future. This is a failure state and if I had a manager doing that to those he was managing, he wouldn't be a manager for long.
- caconym_ 3y ago
  > A lot of these maintainers are already overworked and don't have time to tutor every single person that wants to put kernel contributor on their resume. This seems like a very cynical view of what happened here, and completely unfounded. Obviously these big open-source projects need gatekeepers, but if the gatekeepers often make aspiring contributors feel violated and humiliated rather than offering feedback to get their initial contributions across the finish line, general interest and enthusiasm for contributing are going to dry up. Maybe that's by design, but it doesn't feel sustainable in the long term.
hnfong 3y ago
I looked at the two patches, and TBH the time taken to write the final correct patch would be a fraction of the time needed to explain how to write a better patch. In a company where people are expected to stay around for a while, it may make sense to invest time to bring juniors up to speed. But I wonder whether that's true for OSS projects in general. A person randomly shows up to report and attempt fix a bug in an "obscure"(?) architecture -- this is not a junior who just joined the team, and they have no commitment to stick around to do more contributions. In this case, from a utilitarian perspective, it doesn't seem obvious the maintainer should spend the extra time coaching. This might be a minority view here, but TBH I don't think anyone should be obliged to go out of their way to spend extra time and effort to avoid offending a random person on the Internet.
- tjoff 3y ago
  The time to write the patch is insignificant in this context.
- that_guy_iain 3y ago
  I would say that the author stated he asked for his patch to be accepted so he could be a contributor implies they weren't planning on contributing more. So it doesn't seem like a sceranio where they should spend that extra time.
  - deleted 3y ago
    [deleted]
- kazinator 3y ago
  Not really. 1. Please don't drop cases from your #if CONFIG_PPC32 blocks that are handled on 64 bit. You can use if (IS_ENABLED(CONFIG_PPC32)) instead of #ifdef so the only difference is in the array access. 2. Please remove the FPRINDEX macro; as you note in your commit message, we can assume TS_FPRWIDTH is 1. The maintainer also wrote a long and detailed commit message in his own version of the patch; he had time to explain things. In this exact situation, I would possibly have made the code adjustments myself and written "patch provided by Ariel Miculas". Then send an e-mail saying that I made a few minor tweaks to your patch, here is what it looks like. "patch provided by" tells the truth: that person provided a working patch. It doesn't say that the patch they provided is precisely this one.
arp242 3y ago
What usually happens is that someone sends a patch with a text description of the problem, but no test case. So what I do in those cases is just quickly look at the patch and create a test case first, on the current stable version, to reproduce the bug. This is both so that a test case exists, but also so that I have a deeper understanding of the problem. When that is done, often the fix is simple, and the author's patch sometimes just ... isn't that great, often for entirely understandable reasons, so it's easier to just fix it myself "from scratch". So whose patch is it then? In some cases the patch was useful and I'm "inspired" by it, in other cases I'm not. I tend to err on the side of caution, but I've definitely merged my share of patches where the Author field was, IMHO, kind of a lie. As for code review: it's really time-consuming. And while a few "minor bug fixers" of today become "hard-core contributors" or tomorrow, realistically, most people show up to fix one bug and are never seen again. That's perfectly fine! But it does mean that "teaching" everyone who shows up quickly become an enormous time sink. There's a bit of a trade-off to be made here.
- kazinator 3y ago
  "This change is closely based on a well-considered patch from A. U. Thor <author@example.com>, who also originated the bug report." A. U. Thor can point to that 10 years later and say he or she fixed that; nobody cares if the maintainer changed a few things.
- deleted 3y ago
  [deleted]

jacquesm 3y ago

Not on a security related list though. That's entirely the wrong expectation.

varispeed 3y ago
Security through obscurity? Oh...
Arnavion 3y ago
linuxppc-dev is not a security list.
- jacquesm 3y ago
  It was cc-'d to the security list.
  - deleted 3y ago
    [deleted]
  - Arnavion 3y ago
    On the first patch that Miculas submitted with their gmail address that was not looked at by any of the parties involved in the rest of the story. The subsequent mails to linuxppc-dev where Leroy and Ellerman were involved were not cross-posted AFAICT.
    - jacquesm 3y ago
      Yes, that's correct. But the OP didn't have to CC the security mailing list in the first place and that changed the dynamic considerably. Note that the degree to which this has blown up is disproportional and that besides the fact that Michael Ellerman could have handled this more gracefully the OP did not seek to resolve this out of band, misrepresented the interaction and ignored an olive branch that would have given him exactly what he wanted. The bit where I am in strict disagreement with how Michael Ellerman handled this is actually the reverse of what happened: if the original patch wasn't properly signed off on and the OP was responsive they actually put themselves at risk by re-using the code without proper attribution. This - and that's speculative - probably was because the code identified the essence of the fix but introduced a couple of new problems and because it was so small he may have felt that fixing it and adding a 'Reported-by' tag plus the LKML archive would be sufficient cover for that. But from a strict reading of the LKML guidelines that was the wrong thing to do. And the LKML guidelines themselves should be made clearer with respect to how the various levels of contribution are dealt with because that seems to matter a lot to some people and they need to spell out more clearly that submitting a patch may result in your code ending up in the kernel with LKML as the only visible proof that this is the case. Because the OP may not realize it but that is the essential part: that their submission to LKML ended with a bug in the Linux kernel fixed and there is ample proof to document that and I'm pretty sure that nobody would claim otherwise.

fsdjkflsjfsoij 3y ago

A lot of these maintainers are already overworked and don't have time to tutor every single person that wants to put kernel contributor on their resume. A lot of people have this extremely idealistic view of open source but the reality is that the vast majority of patches you receive will be atrocious and you shouldn't have to tutor every single "potential" contributor. This case is even worse since this issue was posted to a security mailing list which means time to resolution is important.

ChildOfEru 3y ago
Who knows how long this would have been unaddressed without Ariel Miculas initial input. Part of the job of a maintainer is to curate and foster input so the project has a robust community in the future. The maintainer in this case did a miserable job of that.
a_t48 3y ago
On the other hand, to dig yourself out of the hole of “I’m too busy and there’s nobody around to help”, you have to slow down and do this, sometimes.
- bhouston 3y ago
  > to dig yourself out of the hole of “I’m too busy and there’s nobody around to help”, you have to slow down and do this Exactly! If you treat new contributors as crap because you are overworked, you will push away potential long-term collaborators who could share the load thus ensuring you will be working alone far into the future. This is a failure state and if I had a manager doing that to those he was managing, he wouldn't be a manager for long.
caconym_ 3y ago
> A lot of these maintainers are already overworked and don't have time to tutor every single person that wants to put kernel contributor on their resume. This seems like a very cynical view of what happened here, and completely unfounded. Obviously these big open-source projects need gatekeepers, but if the gatekeepers often make aspiring contributors feel violated and humiliated rather than offering feedback to get their initial contributions across the finish line, general interest and enthusiasm for contributing are going to dry up. Maybe that's by design, but it doesn't feel sustainable in the long term.

hnfong 3y ago

I looked at the two patches, and TBH the time taken to write the final correct patch would be a fraction of the time needed to explain how to write a better patch. In a company where people are expected to stay around for a while, it may make sense to invest time to bring juniors up to speed. But I wonder whether that's true for OSS projects in general. A person randomly shows up to report and attempt fix a bug in an "obscure"(?) architecture -- this is not a junior who just joined the team, and they have no commitment to stick around to do more contributions. In this case, from a utilitarian perspective, it doesn't seem obvious the maintainer should spend the extra time coaching. This might be a minority view here, but TBH I don't think anyone should be obliged to go out of their way to spend extra time and effort to avoid offending a random person on the Internet.

tjoff 3y ago
The time to write the patch is insignificant in this context.
that_guy_iain 3y ago
I would say that the author stated he asked for his patch to be accepted so he could be a contributor implies they weren't planning on contributing more. So it doesn't seem like a sceranio where they should spend that extra time.
- deleted 3y ago
  [deleted]
kazinator 3y ago
Not really. 1. Please don't drop cases from your #if CONFIG_PPC32 blocks that are handled on 64 bit. You can use if (IS_ENABLED(CONFIG_PPC32)) instead of #ifdef so the only difference is in the array access. 2. Please remove the FPRINDEX macro; as you note in your commit message, we can assume TS_FPRWIDTH is 1. The maintainer also wrote a long and detailed commit message in his own version of the patch; he had time to explain things. In this exact situation, I would possibly have made the code adjustments myself and written "patch provided by Ariel Miculas". Then send an e-mail saying that I made a few minor tweaks to your patch, here is what it looks like. "patch provided by" tells the truth: that person provided a working patch. It doesn't say that the patch they provided is precisely this one.

arp242 3y ago

What usually happens is that someone sends a patch with a text description of the problem, but no test case. So what I do in those cases is just quickly look at the patch and create a test case first, on the current stable version, to reproduce the bug. This is both so that a test case exists, but also so that I have a deeper understanding of the problem. When that is done, often the fix is simple, and the author's patch sometimes just ... isn't that great, often for entirely understandable reasons, so it's easier to just fix it myself "from scratch". So whose patch is it then? In some cases the patch was useful and I'm "inspired" by it, in other cases I'm not. I tend to err on the side of caution, but I've definitely merged my share of patches where the Author field was, IMHO, kind of a lie. As for code review: it's really time-consuming. And while a few "minor bug fixers" of today become "hard-core contributors" or tomorrow, realistically, most people show up to fix one bug and are never seen again. That's perfectly fine! But it does mean that "teaching" everyone who shows up quickly become an enormous time sink. There's a bit of a trade-off to be made here.

kazinator 3y ago
"This change is closely based on a well-considered patch from A. U. Thor <author@example.com>, who also originated the bug report." A. U. Thor can point to that 10 years later and say he or she fixed that; nobody cares if the maintainer changed a few things.
deleted 3y ago
[deleted]