Messages in this thread | | | Subject | Re: [PATCH v8 3.2.0-rc5 1/9] uprobes: Install and remove breakpoints. | From | Steven Rostedt <> | Date | Wed, 04 Jan 2012 12:12:30 -0500 |
| |
On Wed, 2012-01-04 at 17:51 +0100, Peter Zijlstra wrote:
> > + if (is_register) > > + ret = install_breakpoint(mm, uprobe, vma, vi->vaddr); > > + else > > + remove_breakpoint(mm, uprobe, vi->vaddr); > > + > > + up_read(&mm->mmap_sem); > > + mmput(mm); > > + if (is_register) { > > + if (ret && ret == -EEXIST) > > + ret = 0; > > + if (ret) > > + break; > > + } > > Since you init ret := 0 and remove_breakpoint doesn't change it, this > conditional on is_register is superfluous.
True, but I would argue that this is easier to understand. That is, we only break on a failed install_breakpoint (is_register is set). If I looked at this code and saw:
if (is_register) ret = install_breakpoint() else remove_breakpoint()
[...]
if (ret && ret == -EEXIST) ret = 0; if (ret) break;
I would first think that there might be a bug. That is, we should have a ret = remove_breakpoint().
Thus, I would say, either leave this as is and hope gcc is smart enough to optimize out the if (is_register), or add the comment:
/* ret will always be zero on remove_breakpoint */ if (ret && ret == -EEXIST) ret = 0; if (ret) break;
-- Steve
> > > + } > > + list_for_each_entry_safe(vi, tmpvi, &try_list, probe_list) { > > + list_del(&vi->probe_list); > > + kfree(vi); > > + } > > + return ret; > > +}
| |