2015-03-23 13:36, Pavel Boldin: > Hi Thomas, > > There is by far more than 2 issues, but these are the only ones that > appeared to us. > > The list of the issues that motivated the refactoring: > > 1. Only one error code for every fault (-EFAULT). > 2. Copy-paste code from the `fget' function. > 3. Ambiguous variable names. The `files' variable mean different things > under the same block. This ruins readability of the code. > 4. Overall placing of the all the code inside one `switch/case'. Modern > kernel code style suggests placing these in the inline functions. > 5. Usage of the copy-pasted `close_fd' function code. (I really should > use `sys_close' here though). > > > I can rewrite the refactoring in a set of small patches if this will aid > reviewers. > > The dangerous bug that definitely must be fixed before the next release is > the `struct file*' leakage. I can provide a simple patch for this as a > separate submission and continue working on the refactoring patches. Is > this plan OK for you?
Yes, please separate important fixes and refactoring. Then we'll be able to merge the fixes in 2.0. PS: please answer below the question to ease thread reading. Thanks > On Mon, Mar 23, 2015 at 1:15 PM, Thomas Monjalon <thomas.monjalon at > 6wind.com> > wrote: > > > Hi Pavel, > > > > You forgot the Signed-off-by line. > > > > Huawei, Changchun, any comment on this patch? > > > > 2015-03-18 15:16, Pavel Boldin: > > > The `eventfd_link' module provides an API to "steal" fd from another > > > process had been written with a bug that leaks `struct file' because > > > of the extra reference counter increment and missing `fput' call. > > > > > > The other bug is using another process' `task_struct' without > > incrementing > > > a reference counter. > > > > As there are 2 bugs, you should provide 2 patches. > > > > > Fix these bugs and refactor the module. > > > > Why refactoring along with the bug fixes? > > You'd have more chances to have your fixes integrated in 2.0 without > > refactoring. > > > > Thanks
