Hi,
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.
>
> +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.
> +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?
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.
--
Mindaugas
