Hi, Thank you for your comments.
On 2015/07/17 5:18, Mindaugas Rasiukevicius wrote: > Kengo NAKAHARA <[email protected]> wrote: >> There has been no objection for about 10 months... >> So, I am going to commit this implementation. Here is the rebased >> patch, >> http://netbsd.org/~knakahara/intrctl/intrctl.patch >> >> If there is no objection, I commit this patch after a few days. > > I have not really had time to do a proper review, but from a quick skim: > >> + >> + mutex_enter(&cpu_lock); >> + ih = intr_get_handler(intrid); >> + mutex_exit(&cpu_lock); >> + > > How is cpu_lock supposed to help here? You protect the iteration, but > not the actual handler structure once it is acquired. It does not seem > to be MP-safe. I fix this reading side MP-unsafe issues. >> +int >> +intr_construct_intrids(const kcpuset_t *cpuset, char ***intrids, int *count) >> +{ >> > > Might be worth to put the IDs and their count in a structure rather than > passing as parameters, but that is up to you. I put IDs and number of IDs in a structure, and refactor whole function. >> +intr_list_sysctl(SYSCTLFN_ARGS) >> ... >> + buf = kmem_zalloc(*oldlenp, KM_SLEEP); >> + if (buf == NULL) >> + return ENOMEM; >> + >> + ret = intr_list(buf, *oldlenp); >> + if (ret < 0) >> + return -ret; >> + >> + return copyout(buf, oldp, *oldlenp); >> +} > > This seems like a memory leak (buf is never freed). Also, why negative > error numbers in intr_list() and elsewhere? Because intr_list() returns the size of interrupts list data (used by "intrctl(8) list") on success, and returns -1 * (error code) on fail. > Note: kcpuset_copyin() returns an error code; although you zero the set > on creation, I think it is better to check for kcpuset_copyin() errors. I fix the this error check and above memory leak. Furthermore, I review once more with my co-workers, and fix some issues. Here is the updated patch http://netbsd.org/~knakahara/intrctl/intrctl2.patch Could you comments this patch? Thanks, -- ////////////////////////////////////////////////////////////////////// Internet Initiative Japan Inc. Device Engineering Section, Core Product Development Department, Product Division, Technology Unit Kengo NAKAHARA <[email protected]>
