4 ms·

Comparing both patches, I see a clear difference. While the contribution from the linked article changes the behaviour of both branches of the if...else, the fi

by micw 3y ago

Comparing both patches, I see a clear difference. While the contribution from the linked article changes the behaviour of both branches of the if...else, the final contribution changes only the parth in the "if" branch. I guess, the maintainer had some reason to change the code. Nevertheless, giving credits to the original patch author would be appropriate since the actual fix was almost 100% copied.

deleted 3y ago
[deleted]
deleted 3y ago
[deleted]
ariel-miculas 3y ago
I'm the author of the post. Unfortunately, I cannot link to the original patch I sent (it was very different) because I only sent it to the Linux kernel security mailing list. I wish I had saved it somewhere so I could show it.
- toyg 3y ago
  If your mail client doesn't have a "sent" folder, you need a better mail client :) (Totally with you though: maintainer was a douche. People are the worst.)
- angio 3y ago
  Should be in your "Sent" folder right?
  - ariel-miculas 3y ago
    Thanks, I actually have it in the sent folder of my personal email, I forgot about this. How can I share the email thread with you?
    - ariel-miculas 3y ago
      Ok I've forwarded the email thread: https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg221962.html https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/m...
      - dfox 3y ago
        Forwarding a random email conversation from a year+ ago without any context to public mailing list that is read who knows how many people just to get it into an archive so that you can link to it in HN comment does not really help your case.
        dfox 3y ago
        And if you care so much about getting your code into the kernel you should consider replying to this: https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg221972.html https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/m... On a related note Christophe Leroy should be applauded for his calm and matter of fact response. I don’t know if I would be capable of that.
    - Faaak 3y ago
      You can export the eml of the sent mail
micw 3y ago
Same for the original patch from the mailing conversation above. The original results in (reformatted) #ifdef CONFIG_PPC32 *data = ((unsigned int *)child->thread.fp_state.fpr)[FPRINDEX(index)]; #else flush_fp_to_thread(child); if (fpidx < (PT_FPSCR - PT_FPR0)) memcpy(data, &child->thread.TS_FPR(fpidx), sizeof(long)); else *data = child->thread.fp_state.fpscr; #endif while the final patch is flush_fp_to_thread(child); if (fpidx < (PT_FPSCR - PT_FPR0)) { if (IS_ENABLED(CONFIG_PPC32)) *data = ((u32 *)child->thread.fp_state.fpr)[fpidx]; else memcpy(data, &child->thread.TS_FPR(fpidx), sizeof(long)); } else *data = child->thread.fp_state.fpscr; So there is actually a difference between both solutions. The original has neither the flush_fp_to_thread not the else-condition for the PPC32 architecture. I cannot say which is right/wrong/better but it's definitively a different result.
- l-albertovich 3y ago
  There's a clear logical difference in those two patches and I can't blame the maintainer for going with his version. Attribution should be given though, regardless of the bug OPs patch introduced the actual line that fixes the issue is clearly the same thing with the appropriate coding style. As a maintainer, I can understand his reasoning but TBH it would've been easier and more respectful to just reply with the proper code snippet, having the contributor submit it (if he agreed) and then giving them credit. Honestly, the comment section of this thread sucks, the level of self righteousness, "aggression" and gatekeeping is ridiculously uncalled for.
  - rolandog 3y ago
    Agreed. You basically get paid in reputation, trustworthiness, and maybe even beer/brownie/pizza points. I also think that public interactions offer a preview of how easy (or difficult) it will be to work with someone on a project... we're not experts on every domain of knowledge, so the best bet is to approach interactions with a bit of humility and respect. If this were an AITA reddit thread, ESH (a little bit).
- deleted 3y ago
  [deleted]
queuebert 3y ago
As an old C programmer, I'd be wary of code sent to me with a bunch of custom macros. That might indicate a certain immaturity with C. Maintaining a large code base is part fire suppression and part avoiding name collisions.

by micw 3y ago

deleted 3y ago

[deleted]

deleted 3y ago

[deleted]

ariel-miculas 3y ago

I'm the author of the post. Unfortunately, I cannot link to the original patch I sent (it was very different) because I only sent it to the Linux kernel security mailing list. I wish I had saved it somewhere so I could show it.

toyg 3y ago
If your mail client doesn't have a "sent" folder, you need a better mail client :) (Totally with you though: maintainer was a douche. People are the worst.)
angio 3y ago
Should be in your "Sent" folder right?
- ariel-miculas 3y ago
  Thanks, I actually have it in the sent folder of my personal email, I forgot about this. How can I share the email thread with you?
  - ariel-miculas 3y ago
    Ok I've forwarded the email thread: https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg221962.html https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/m...
    - dfox 3y ago
      Forwarding a random email conversation from a year+ ago without any context to public mailing list that is read who knows how many people just to get it into an archive so that you can link to it in HN comment does not really help your case.
      - dfox 3y ago
        And if you care so much about getting your code into the kernel you should consider replying to this: https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg221972.html https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/m... On a related note Christophe Leroy should be applauded for his calm and matter of fact response. I don’t know if I would be capable of that.
  - Faaak 3y ago
    You can export the eml of the sent mail

micw 3y ago

Same for the original patch from the mailing conversation above. The original results in (reformatted) #ifdef CONFIG_PPC32 *data = ((unsigned int *)child->thread.fp_state.fpr)[FPRINDEX(index)]; #else flush_fp_to_thread(child); if (fpidx < (PT_FPSCR - PT_FPR0)) memcpy(data, &child->thread.TS_FPR(fpidx), sizeof(long)); else *data = child->thread.fp_state.fpscr; #endif while the final patch is flush_fp_to_thread(child); if (fpidx < (PT_FPSCR - PT_FPR0)) { if (IS_ENABLED(CONFIG_PPC32)) *data = ((u32 *)child->thread.fp_state.fpr)[fpidx]; else memcpy(data, &child->thread.TS_FPR(fpidx), sizeof(long)); } else *data = child->thread.fp_state.fpscr; So there is actually a difference between both solutions. The original has neither the flush_fp_to_thread not the else-condition for the PPC32 architecture. I cannot say which is right/wrong/better but it's definitively a different result.

l-albertovich 3y ago
There's a clear logical difference in those two patches and I can't blame the maintainer for going with his version. Attribution should be given though, regardless of the bug OPs patch introduced the actual line that fixes the issue is clearly the same thing with the appropriate coding style. As a maintainer, I can understand his reasoning but TBH it would've been easier and more respectful to just reply with the proper code snippet, having the contributor submit it (if he agreed) and then giving them credit. Honestly, the comment section of this thread sucks, the level of self righteousness, "aggression" and gatekeeping is ridiculously uncalled for.
- rolandog 3y ago
  Agreed. You basically get paid in reputation, trustworthiness, and maybe even beer/brownie/pizza points. I also think that public interactions offer a preview of how easy (or difficult) it will be to work with someone on a project... we're not experts on every domain of knowledge, so the best bet is to approach interactions with a bit of humility and respect. If this were an AITA reddit thread, ESH (a little bit).
deleted 3y ago
[deleted]

queuebert 3y ago

As an old C programmer, I'd be wary of code sent to me with a bunch of custom macros. That might indicate a certain immaturity with C. Maintaining a large code base is part fire suppression and part avoiding name collisions.