Mailing List Archive

[RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target
From: Keiichi KII <k-keiichi@bx.jp.nec.com>

We add ioctls for adding/removing target.
If we use NETCONSOLE_ADD_TARGET ioctl,
we can dynamically add netconsole target.
If we use NETCONSOLE_REMOVE_TARGET ioctl,
we can dynamically remoe netconsole target.

We attach a sample program for ioctl.

Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com>
Signed-off-by: Takayoshi Kochi <t-kochi@bq.jp.nec.com>
---
/*
* This software is a sample program for ioctl of netconsole.
* You can add/remove netconsole port by using this software.
*
* Copyright (C) 2007 NEC Corporation

* Created by Keiichi KII <k-keiichi@bx.jp.nec.com>

* This software is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License as
* published by the Free Software Foundation; either version 2 or
* (at your option) any later version.
*/

#include <stdio.h>
#include <unistd.h>
#include <string.h>
#include <stdlib.h>
#include <stropts.h>
#include <fcntl.h>
#include <arpa/inet.h>
#include <net/if.h>
#include <linux/if_ether.h>
#include <linux/netconsole.h>

#define NETCONSOLE_DEV_NAME "/dev/netconsole"
#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))

struct command {
char *name;
char *options;
int (*handle_command)(struct command* command, int argc, char* argv[]);
void (*usage)(char *msg);
};

extern char *optarg;
extern int opterr, optind, errno;

static void generic_usage(char *msg) {
fprintf(stderr, "Usage : netconfig command [option] [args]\n");
fprintf(stderr, "command: add remove help\n");
exit(-1);
}

static int handle_command_add(struct command* command, int argc, char** argv)
{
int i, fd, ch;
unsigned int address;
unsigned char mac[ETH_ALEN];
struct netconsole_request req = {
.netdev_name = "eth0",
.local_port = 6665,
.remote_port = 6666,
.remote_mac = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
};

while ((ch = getopt(argc, argv, command->options)) != -1) {
switch (ch) {
case 'p':
req.local_port = atoi(optarg);
break;
case 's':
address = inet_addr(optarg);
if (address == -1)
(*command->usage)("invlid IP address!\n");
req.local_ip = address;
break;
case 'h':
default:
(*command->usage)(NULL);
}
}
argc -= optind;
argv += optind;

if (argc < 3 || argc > 4)
(*command->usage)(NULL);

memcpy(req.netdev_name, argv[0], IFNAMSIZ);
address = inet_addr(argv[1]);
if (address == -1)
(*command->usage)("invlid IP address!\n");
req.remote_ip = address;
req.remote_port = atoi(argv[2]);
if (argc == 4) {
i = 0;
mac[i++] = strtol(argv[3], NULL, 16);
while ((argv[3] = strchr(argv[3], ':')) != NULL) {
argv[3]++;
mac[i++] = strtol(argv[3], NULL, 16);
}
if (i != ETH_ALEN)
(*command->usage)("Invalid MAC address!\n");
memcpy(req.remote_mac, mac, ETH_ALEN);
}

fd = open(NETCONSOLE_DEV_NAME, O_RDWR);
if (fd < 0) {
fprintf(stderr, "cannot open device" NETCONSOLE_DEV_NAME "\n");
return -1;
}

if(ioctl(fd, NETCON_ADD_TARGET, &req) != 0)
perror("add");
close(fd);

return 0;
}

static void usage_add(char *msg)
{
if (msg != NULL)
fprintf(stderr, "%s", msg);
fprintf(stderr, "Usage : netconfig add [-options] dev_name remote_ip "
"remote_port [remote_mac]\n");
fprintf(stderr, "options:\n");
fprintf(stderr, " -p local_port :local port number\n");
fprintf(stderr, " -s local_up :local IP address\n");
exit(-1);
}

static int handle_command_remove(struct command *command,
int argc, char** argv)
{
int fd, id, ch;

while ((ch = getopt(argc, argv, command->options)) != -1) {
switch (ch) {
case 'h':
default:
(*command->usage)(NULL);
}
}
argc -= optind;
argv += optind;

if (argc != 1)
(*command->usage)(NULL);

id = atoi(argv[0]);
fd = open(NETCONSOLE_DEV_NAME, O_RDWR);
if (fd < 0) {
fprintf(stderr, "can't open device " NETCONSOLE_DEV_NAME "\n");
return -1;
}
if(ioctl(fd, NETCON_REMOVE_TARGET, &id) != 0)
perror("remove");
close(fd);

return 0;
}

static void usage_remove(char *msg)
{
fprintf(stderr, "Usage : netconfig remove id\n");
exit(-1);
}

static int handle_command_help(struct command *command, int argc, char** argv)
{
(*command->usage)(NULL);

return 0;
}

static void usage_help(char *msg)
{
generic_usage(NULL);
}

static struct command s_commands[] = {
{"add", "hp:s:", handle_command_add, usage_add},
{"remove", "hp:", handle_command_remove, usage_remove},
{"help", NULL, handle_command_help, usage_help},
};

int main(int argc, char* argv[])
{
int i;
struct command *command = NULL;

for(i = 0; i < ARRAY_SIZE(s_commands); i++) {
if (argc <= 1)
break;
if (strcmp(s_commands[i].name, argv[1]) == 0) {
command = &s_commands[i];
break;
}
}

if (command == NULL)
generic_usage(NULL);
argc--;
argv++;

if (command->handle_command)
(*command->handle_command)(command, argc, argv);
else {
fprintf(stderr, "function handle_command() isn't set.");
exit(-1);
}
return 0;
}
Index: mm/fs/compat_ioctl.c
===================================================================
--- mm.orig/fs/compat_ioctl.c
+++ mm/fs/compat_ioctl.c
@@ -114,6 +114,8 @@
#include <linux/dvb/video.h>
#include <linux/lp.h>

+#include <linux/netconsole.h>
+
static int do_ioctl32_pointer(unsigned int fd, unsigned int cmd,
unsigned long arg, struct file *f)
{
@@ -3489,6 +3491,12 @@ HANDLE_IOCTL(LPSETTIMEOUT, lp_timeout_tr

IGNORE_IOCTL(VFAT_IOCTL_READDIR_BOTH32)
IGNORE_IOCTL(VFAT_IOCTL_READDIR_SHORT32)
+
+#ifdef CONFIG_NETCONSOLE_DYNCON
+/* netconsole */
+COMPATIBLE_IOCTL(NETCON_ADD_TARGET)
+ULONG_IOCTL(NETCON_REMOVE_TARGET)
+#endif
};

#define IOCTL_HASHSIZE 256
Index: mm/include/linux/netconsole.h
===================================================================
--- /dev/null
+++ mm/include/linux/netconsole.h
@@ -0,0 +1,18 @@
+#ifndef __NETCONSOLE_H
+#define __NETCONSOLE_H
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+struct netconsole_request {
+ __u32 id;
+ __u8 netdev_name[IFNAMSIZ];
+ __u8 remote_mac[ETH_ALEN];
+ __u32 local_ip, remote_ip;
+ __u16 local_port, remote_port;
+};
+
+#define NETCON_ADD_TARGET _IOW(0xAE, 4, struct netconsole_request)
+#define NETCON_REMOVE_TARGET _IOW(0xAE, 5, int)
+
+#endif /* __NETCONSOLE_H */
Index: mm/drivers/net/netconsole.c
===================================================================
--- mm.orig/drivers/net/netconsole.c
+++ mm/drivers/net/netconsole.c
@@ -47,6 +47,7 @@
#include <linux/netpoll.h>
#include <linux/miscdevice.h>
#include <linux/inet.h>
+#include <linux/netconsole.h>

MODULE_AUTHOR("Maintainer: Matt Mackall <mpm@selenic.com>");
MODULE_DESCRIPTION("Console driver for network interfaces");
@@ -75,6 +76,8 @@ struct netconsole_target {
struct netpoll np;
};

+static struct tty_driver *netconsole_tty_driver;
+
static LIST_HEAD(target_list);
static DEFINE_SPINLOCK(target_list_lock);
static DECLARE_MUTEX(netdev_change_sem);
@@ -442,6 +445,73 @@ static int netconsole_event(struct notif
static struct notifier_block netconsole_notifier = {
.notifier_call = netconsole_event,
};
+
+static int netconsole_open(struct tty_struct *tty, struct file *file)
+{
+ return 0;
+}
+
+static int netconsole_ioctl(struct tty_struct *tty, struct file *file,
+ unsigned int cmd, unsigned long arg)
+{
+ int id, count;
+ char config[256];
+ char *cur;
+ struct netconsole_request req;
+ struct netconsole_target *nt, *tmp;
+ void __user *argp = (void __user *)arg;
+
+ switch (cmd) {
+ case NETCON_ADD_TARGET:
+ printk(KERN_INFO "netconsole: cmd=NETCON_ADD_TARGET\n");
+ if (copy_from_user(&req, argp, sizeof(req)))
+ return -EFAULT;
+ cur = config;
+ count = sprintf(cur, "%d@", req.local_port);
+ cur += count;
+ if (req.local_ip)
+ count = sprintf(cur, "%d.%d.%d.%d/",
+ NIPQUAD(req.local_ip));
+ else
+ count = sprintf(cur, "/");
+ cur += count;
+ count = sprintf(cur, "%s,", req.netdev_name);
+ cur += count;
+ count = sprintf(cur, "%d@", req.remote_port);
+ cur += count;
+ count = sprintf(cur, "%d.%d.%d.%d/",
+ NIPQUAD(req.remote_ip));
+ cur += count;
+ count = sprintf(cur, "%02x:%02x:%02x:%02x:%02x:%02x",
+ req.remote_mac[0], req.remote_mac[1],
+ req.remote_mac[2], req.remote_mac[3],
+ req.remote_mac[4], req.remote_mac[5]);
+ printk(KERN_INFO "count = %d config=[%s]\n", count, config);
+ if (add_target(config))
+ return -EINVAL;
+ break;
+ case NETCON_REMOVE_TARGET:
+ printk(KERN_INFO "netconsole: cmd=NETCON_REMOVE_TARGET\n");
+ if (copy_from_user(&id, argp, sizeof(int)))
+ return -EFAULT;
+ printk(KERN_INFO "netconsole: id=%d\n", id);
+ list_for_each_entry_safe(nt, tmp, &target_list, list)
+ if (nt->id == id) {
+ kobject_unregister(&nt->obj);
+ break;
+ }
+ break;
+ default:
+ return -ENOTTY;
+ }
+
+ return 0;
+}
+
+static struct tty_operations netconsole_ops = {
+ .open = netconsole_open,
+ .ioctl = netconsole_ioctl,
+};
#endif /* CONFIG_NETCONSOLE_DYNCON */

static void write_msg(struct console *con, const char *msg, unsigned int len)
@@ -510,6 +580,8 @@ static void cleanup_netconsole(void)
unregister_netdevice_notifier(&netconsole_notifier);
if (miscdev_configured)
misc_deregister(&netconsole_miscdev);
+ tty_unregister_driver(netconsole_tty_driver);
+ put_tty_driver(netconsole_tty_driver);
#else
unregister_console(&netconsole);
netpoll_cleanup(&np);
@@ -521,6 +593,7 @@ static int __init init_netconsole(void)
char *tmp = config;
#ifdef CONFIG_NETCONSOLE_DYNCON
char *p;
+ int result;

if (misc_register(&netconsole_miscdev)) {
printk(KERN_ERR
@@ -531,6 +604,27 @@ static int __init init_netconsole(void)
} else
miscdev_configured = 1;

+ netconsole_tty_driver = alloc_tty_driver(1);
+ if (!netconsole_tty_driver)
+ return -ENOMEM;
+ netconsole_tty_driver->owner = THIS_MODULE;
+ netconsole_tty_driver->name = "netcon";
+ netconsole_tty_driver->driver_name = "netconsole";
+ netconsole_tty_driver->major = 0;
+ netconsole_tty_driver->minor_start = 0;
+ netconsole_tty_driver->num = 1;
+ netconsole_tty_driver->type = TTY_DRIVER_TYPE_CONSOLE;
+ tty_set_operations(netconsole_tty_driver, &netconsole_ops);
+
+ if ((result = tty_register_driver(netconsole_tty_driver))) {
+ printk(KERN_ERR "failed to register netconsole tty driver"
+ " errno=%d\n", result);
+ put_tty_driver(netconsole_tty_driver);
+ if (miscdev_configured)
+ misc_deregister(&netconsole_miscdev);
+ return result;
+ }
+
register_netdevice_notifier(&netconsole_notifier);
register_console(&netconsole);
if (!strlen(config)) {

--
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target [ In reply to ]
Hi Keiichi,

On 6/13/07, Keiichi KII <k-keiichi@bx.jp.nec.com> wrote:
> From: Keiichi KII <k-keiichi@bx.jp.nec.com>
>
> We add ioctls for adding/removing target.
> If we use NETCONSOLE_ADD_TARGET ioctl,
> we can dynamically add netconsole target.
> If we use NETCONSOLE_REMOVE_TARGET ioctl,
> we can dynamically remoe netconsole target.

*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 :-)

Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target [ In reply to ]
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.

Thanks
--
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target [ In reply to ]
Hi again Keiichi,

On 6/19/07, Keiichi KII <k-keiichi@bx.jp.nec.com> 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target [ In reply to ]
Hello Satyam,

> 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)?

You're right.
At first, I implemented ioctls to misc device because of using misc sysfs.
But, Andrew Morton said "Using an ioctl() against a miscdev is rather
untypical for networking.". So, I implemented ioclts to tty_driver.

> 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.

It was for informational pupose. But, if we used symlink to the net_device
kobject in sysfs, we could easily keep up with changing network device name by
changing symbolic link.
In the case of configfs, Do we use config_item related to the network interface
because the configfs doesn't have symlink that refers to net_device kobject
(e.g. "network_interface" in configfs, "network_interface" value is "eth0")?

I'm going to search configfs and modify interface to configfs.

Thanks
--
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH -mm take5 6/7] add ioctls for adding/removing target [ In reply to ]
Hi Keiichi,

> > 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.
>
> It was for informational pupose. But, if we used symlink to the net_device
> kobject in sysfs, we could easily keep up with changing network device name by
> changing symbolic link.

In that case (because it is non-essential) we can drop that link entirely.
(This means the associated code for the temporary modify_list, the
extra mutex and the kasprintf() stuff can also be dropped too!) I guess
the netconsole_event() notifier could now become simply:

if (event == NETDEV_CHANGENAME) {
spin_lock_irqsave(&target_list_lock, ...);
list_for_each_entry(nt, ..., &target_list, ...)
if (nt->np.dev == dev)
strcpy(nt->np.dev_name, dev->name);
spin_unlock_irqrestore(&target_list_lock, ...);
}

> In the case of configfs, Do we use config_item related to the network interface
> because the configfs doesn't have symlink that refers to net_device kobject
> (e.g. "network_interface" in configfs, "network_interface" value is "eth0")?

Yes, that's a good idea, this way we continue to give the information we
were giving previously. We could do this by defining another configfs
attribute of the target's config_item which could then be a proxy for
the char dev_name[IFNAMSIZ] member of the target's underlying netpoll
structure. (this attribute would naturally be read-only from userspace;
sort of similar to what you're doing for the local_mac attribute currently).

Please feel free to discuss anything else regarding this with me off-list,
if you want.

Thanks,
Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/