Hi, On 2015/08/11 10:59, Kengo NAKAHARA wrote: > Hi, > > On 2015/07/23 19:44, Kengo NAKAHARA wrote: >> 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? > > There have been no objection for about 3 weeks. > I rebase and modify a little. Here is the updated patch. > http://netbsd.org/~knakahara/intrctl/intrctl3.patch > > Could you comment this patch? > If there is no objection, I will commit this patch after a few days or > weeks.
I committed this patch. Thanks, -- ////////////////////////////////////////////////////////////////////// Internet Initiative Japan Inc. Device Engineering Section, Core Product Development Department, Product Division, Technology Unit Kengo NAKAHARA <[email protected]>
