Re: [RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target

[Satyam Sharma]

Hi again Keiichi,

On 6/19/07, Keiichi KII <k-keiichi@xxxxxxxxxxxxx> wrote:
> Hello Satyam,
>
> > *ugh*. I was wondering what a show-stopper this particular patch
> > was -- introduces a couple of ioctl()'s, exports a new structure to
> > userspace, adds a hitherto-unneeded header file, brings in
> > tty_struct/tty_operations and ends up adding so much complexity/
> > bloat to netconsole.c. Not only that, it must live together (and
> > side-by-side) with the sysfs interface also, because the two of them
> > do different things: sysfs to be able to modify target parameters at
> > run-time and the ioctl()'s to dynamically add/remove targets. We
> > can't really mkdir(2) or rmdir(2) in sysfs so the ioctl()'s are needed.
> >
> > So may I suggest:
> >
> > Just lose *both* the sysfs and ioctl() interfaces and use _configfs_.
> > It is *precisely* the thing you need in your driver here -- the ability
> > to create / destroy kernel objects (or config_items in configfs lingo)
> > from _userspace_ via simple mkdir(2) and rmdir(2). And configfs
> > makes changing multiple configurable parameters atomically trivial
> > too, via rename(2) ... not to mention a sysfs+ioctls -> configfs
> > conversion would help your patchset lose some weight too :-)
>
> Stephen Hemminger previously advised me about the user interface such as
> the following messages.
>
> > Some other speculations:
> > 1. Would it be possible to add ioctl's to /dev/console? This would be more in
> >     keeping with older Unix style model.
> >
> > 2. Using sysfs makes sense if there is a device object that exists to
> >    add the sysfs attributes to.
> >
> > 3. Procfs is handy for summary type tables.
> >
> > 4. Netlink does feel like overkill for this. Although newer generic netlink
> >    makes it easier.
>
> So, I implemented ioctls to add/remove port like this patch on the tty driver.
> But I'm going to search configfs. Thank you for you information.

Hmm, I might've missed this thread, but my opinion on the
alternatives, fwiw:

1. I think adding new ioctl's to the kernel are generally disliked for
obvious reasons. Perhaps Stephen meant to add some generic
ioctl's above (and not separate ones specially implemented for
the dynamically reconfigurable netconsole driver)?

2. sysfs: In fact after reading Stephen's comment above I now noticed/
realized that we're introducing the misc device thing _specifically_ to
be able to attach kobjects for our use in the /sys/class/misc/
namespace? IMHO all such stuff (and the stuff we're adding to be able
to export the ioctl interface) would become unnecessary / redundant if
we switch to configfs, thus making this patchset quite small.

3. Again, as Stephen mentions, procfs interfaces are generally used
for different cases ... personally, I wouldn't even consider a procfs
interface for our case here :-)

4. Yup, netlink is clearly overkill. Also, I think that would require a
special application on the userspace side to talk over the netlink
socket, so we lose the ability to "echo > " from shell ... again,
netlink isn't _quite_ the thing for our case here, IMHO.

Please do consider configfs. Note that we'll have to lose the sysfs
symlink from your target's kobject to the kobject of the ethernet
device if we switch to configfs, but was that symlink needed for
some essential functionality or was it simply for informational
purpose? IMHO, this patchset only needs to bring in functionality
to be able to create, destroy, and modify netconsole targets at
run-time, and all these reconfiguration tasks would be handled
quite well by configfs, AFAICT.

Thanks again,
Satyam

PS: I really liked this patchset (and the idea behind it), and was
thinking of doing a configfs conversion myself, if you hadn't
replied :-) configfs has similar interfaces / structures as sysfs,
so it shouldn't really be too time-consuming.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/