Messages in this thread | | | Date | Tue, 30 Sep 2008 12:08:32 +0300 | From | Avi Kivity <> | Subject | Re: [PATCH 1/3] ioctl: generic ioctl dispatcher |
| |
Andi Kleen wrote: > Avi Kivity <avi@redhat.com> writes: > > >> +long dispatch_ioctl(struct inode *inode, struct file *filp, >> + unsigned cmd, unsigned long arg, >> + const struct ioctl_handler *handlers, >> + long (*fallback)(const struct ioctl_arg *arg)) >> > > The basic idea is good, but i don't like the proliferation of callbacks, > which tends to make complicated code and is ugly for simple code > (which a lot of ioctls are) > >
If the simple calls mostly don't use the argument as a pointer, they are better off using a plain switch. For my own code, I usually leave the boilerplate within the switch and the app-specific code in a separate function anyway, so there's no big change in style.
The main motivation here was the extensibility (patch 2), which becomes much more difficult with a switch.
> How about you make it return an number that can index a switch() instead? > Then everything could be still kept in the same function. > >
We need to execute code both before and after the handler, so it would look pretty ugly:
long my_ioctl_handler(...) { struct ioctl_arg iarg; ... long ret;
ret = dispatch_ioctl_begin(&iarg, ...); if (ret < 0) return ret; switch (ret) { case _IOC_KEY(MY_IOCTL): // your stuff goes here break; ... } dispatch_ioctl_end(&iarg, ret); return ret; }
The only clean way to do this without callbacks is with constructors/destructors, but we don't have those in C.
-- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
| |