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.
Thanks,
--
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.
Device Engineering Section,
Core Product Development Department,
Product Division,
Technology Unit
Kengo NAKAHARA <[email protected]>
