Messages in this thread | | | Subject | Re: [PATCH v4 1/7] x86/fsgsbase/64: Introduce FS/GS base helper functions | From | Andy Lutomirski <> | Date | Fri, 22 Jun 2018 09:08:59 -0700 |
| |
> On Jun 22, 2018, at 8:39 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > >> On Fri, 22 Jun 2018, Andy Lutomirski wrote: >> On Fri, Jun 22, 2018 at 7:28 AM Thomas Gleixner <tglx@linutronix.de> wrote: >>>> +unsigned long read_task_fsbase(struct task_struct *task) >>>> +{ >>>> + unsigned long fsbase; >>>> + >>>> + if (task == current) { >>>> + fsbase = read_fsbase(); >>>> + } else { >>>> + /* >>>> + * XXX: This will not behave as expected if called >>>> + * if fsindex != 0. This preserves an existing bug >>>> + * that will be fixed. >>> >>> I'm late to this party, but let me ask the obvious question: >>> >>> Why is the existing bug not fixed as the first patch in the series? >> >> IIRC that was how I did it in the old version of this code. I think >> it did it because it was less messy to fix the bug after cleaning up >> the code, but I could be remembering wrong. > > Fair enough. Though the general rule is: Fix bugs first and then do > features, unless you really need the extra step to fix it proper. > > Now in that case the real question is whether this is a bug or just a > slight incorrectness which has no practical impact. If it's the latter, > then introduce the new function which does the right thing first and make > the new fs/gs base functions use it without having a blurb about preserving > bugs.
The idea was to have one patch that was intended to have no observable effect (pure refactor) and another to change behavior in an easily reviewable way. I should probably not have used the word bug :)
> > Thanks, > > tglx > > >
| |