Re: [RFC][PATCH -mm 3/5] add interface for netconsole using sysfs

2006-12-25 Thread Keiichi KII

Thank you for your replies and reviews.

I will follow your advices.


 static LIST_HEAD(target_list);
 
 static DEFINE_SPINLOCK(target_list_lock);
 
+static ssize_t show_local_ip(struct netconsole_target *nt, char *buf)

+{
+   return sprintf(buf, "%d.%d.%d.%d\n", HIPQUAD(nt->np.local_ip));


I don't understand the use of HIPQUAD() here instead of
NIPQUAD().  Explain?

Also, NIPQUAD_FMT (in kernel.h) uses "%u.%u.%u.%u".
This should probably be the same.
Or just use:NIPQUAD_FMT "\n"


IP address is stored in the form of host byte order in netpoll structure.
So, You can't use NIPQUAD to follow the current implementation of netpoll.

--
Keiichi KII
NEC Corporation OSS Promotion Center
E-mail: [EMAIL PROTECTED]


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
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 3/5] add interface for netconsole using sysfs

2006-12-23 Thread Randy Dunlap
On Fri, 22 Dec 2006 21:14:36 +0900 Keiichi KII wrote:

> From: Keiichi KII <[EMAIL PROTECTED]>
> 
> ---
> [changes]
> 1. expand macro code as far as possible.
> 2. follow kernel coding style.
> 3. print proper output messeage.
> 4. attach proper label for printk.
> 5. integrate netpoll_lock and netcon_target_list_lock into common spinlock.
> 6. return proper error value.
> 
> --- linux-mm/drivers/net/netconsole.c 2006-12-22 20:54:54.431673500 +0900
> +++ enhanced-netconsole/drivers/net/netconsole.c.sysfs2006-12-22 
> 16:12:47.925833000 +0900
> @@ -56,18 +58,234 @@ MODULE_PARM_DESC(netconsole, " netconsol
>  
>  struct netconsole_target {
>   struct list_head list;
> + struct kobject obj;
>   int id;
>   struct netpoll np;
>  };
>  
> +#define MAX_PRINT_CHUNK 1000
> +#define CONFIG_SEPARATOR ":"
> +#define MAC_ADDR_DIGIT 6

Can you use ETH_ALEN from if_ether.h instead of MAC_ADDR_DIGIT ?

> +
> +struct target_attr {
> + struct attribute attr;
> + ssize_t (*show)(struct netconsole_target*, char*);
> + ssize_t (*store)(struct netconsole_target*, const char*,
> +  size_t count);
> +};
> +
>  static int add_target(char* target_config);
> +static void setup_target_sysfs(struct netconsole_target *nt);
>  static void cleanup_netconsole(void);
>  static void delete_target(struct netconsole_target *nt);
>  
> +static int miscdev_configured = 0;

Don't init static data to 0 or NULL.
It's done for us.

>  static LIST_HEAD(target_list);
>  
>  static DEFINE_SPINLOCK(target_list_lock);
>  
> +static ssize_t show_local_ip(struct netconsole_target *nt, char *buf)
> +{
> + return sprintf(buf, "%d.%d.%d.%d\n", HIPQUAD(nt->np.local_ip));

I don't understand the use of HIPQUAD() here instead of
NIPQUAD().  Explain?

Also, NIPQUAD_FMT (in kernel.h) uses "%u.%u.%u.%u".
This should probably be the same.
Or just use:NIPQUAD_FMT "\n"

> +}
> +
> +static ssize_t show_remote_ip(struct netconsole_target *nt, char *buf)
> +{
> + return sprintf(buf, "%d.%d.%d.%d\n", HIPQUAD(nt->np.remote_ip));
> +}
> +
> +
> +static ssize_t store_remote_mac(struct netconsole_target *nt, const char 
> *buf,
> +size_t count)
> +{
> + unsigned char input_mac[MAC_ADDR_DIGIT] =
> + {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
> + const char *cur = buf;
> + char *delim;
> + int i = 0;
> +
> + for(i=0; i < MAC_ADDR_DIGIT; i++) {

for (i = 0; i < MAC_ADDR_DIGIT; i++) {

> + input_mac[i] = simple_strtol(cur, NULL, 16);
> + if (i != MAC_ADDR_DIGIT - 1 &&
> + (delim = strchr(cur, ':')) == NULL) {
> + return -EINVAL;

No braces on one-line "blocks", please.

> + }
> + cur = delim + 1;
> + }
> + spin_lock(&target_list_lock);
> + memcpy(nt->np.remote_mac, input_mac, MAC_ADDR_DIGIT);
> + spin_unlock(&target_list_lock);
> +
> + return count;
> +}
> +
> +

---
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[RFC][PATCH -mm 3/5] add interface for netconsole using sysfs

2006-12-22 Thread Keiichi KII
From: Keiichi KII <[EMAIL PROTECTED]>

This patch contains the following changes.

create a sysfs entry for netconsole in /sys/class/misc.
This entry has elements related to netconsole as follows.
You can change configuration of netconsole(writable attributes such as IP
address, port number and so on) and check current configuration of netconsole.

-+- /sys/class/misc/
 |-+- netconsole/
   |-+- port1/
   | |--- id  [r--r--r--]  unique port id
   | |--- remove  [-w---]  if you write something to "remove",
   | | this port is removed.
   | |--- dev_name[r--r--r--]  network interface name
   | |--- local_ip[rw-r--r--]  source IP to use, writable
   | |--- local_port  [rw-r--r--]  source port number for UDP packets, writable
   | |--- local_mac   [r--r--r--]  source MAC address
   | |--- remote_ip   [rw-r--r--]  port number for logging agent, writable
   | |--- remote_port [rw-r--r--]  IP address for logging agent, writable
   |  remote_mac  [rw-r--r--]  MAC address for logging agent, writable
   |--- port2/
   |--- port3/
   ...

Signed-off-by: Keiichi KII <[EMAIL PROTECTED]>
---
[changes]
1. expand macro code as far as possible.
2. follow kernel coding style.
3. print proper output messeage.
4. attach proper label for printk.
5. integrate netpoll_lock and netcon_target_list_lock into common spinlock.
6. return proper error value.

--- linux-mm/drivers/net/netconsole.c   2006-12-22 20:54:54.431673500 +0900
+++ enhanced-netconsole/drivers/net/netconsole.c.sysfs  2006-12-22 
16:12:47.925833000 +0900
@@ -45,6 +45,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 MODULE_AUTHOR("Maintainer: Matt Mackall <[EMAIL PROTECTED]>");
 MODULE_DESCRIPTION("Console driver for network interfaces");
@@ -56,18 +58,234 @@ MODULE_PARM_DESC(netconsole, " netconsol
 
 struct netconsole_target {
struct list_head list;
+   struct kobject obj;
int id;
struct netpoll np;
 };
 
+#define MAX_PRINT_CHUNK 1000
+#define CONFIG_SEPARATOR ":"
+#define MAC_ADDR_DIGIT 6
+
+struct target_attr {
+   struct attribute attr;
+   ssize_t (*show)(struct netconsole_target*, char*);
+   ssize_t (*store)(struct netconsole_target*, const char*,
+size_t count);
+};
+
 static int add_target(char* target_config);
+static void setup_target_sysfs(struct netconsole_target *nt);
 static void cleanup_netconsole(void);
 static void delete_target(struct netconsole_target *nt);
 
+static int miscdev_configured = 0;
+
 static LIST_HEAD(target_list);
 
 static DEFINE_SPINLOCK(target_list_lock);
 
+static ssize_t show_id(struct netconsole_target *nt, char *buf)
+{
+   return sprintf(buf, "%d\n", nt->id);
+}
+
+static ssize_t show_dev_name(struct netconsole_target *nt, char *buf)
+{
+   return sprintf(buf, "%s\n", nt->np.dev_name);
+}
+
+static ssize_t show_local_port(struct netconsole_target *nt, char *buf)
+{
+   return sprintf(buf, "%d\n", nt->np.local_port);
+}
+
+static ssize_t show_remote_port(struct netconsole_target *nt, char *buf)
+{
+   return sprintf(buf, "%d\n", nt->np.remote_port);
+}
+
+static ssize_t show_local_ip(struct netconsole_target *nt, char *buf)
+{
+   return sprintf(buf, "%d.%d.%d.%d\n", HIPQUAD(nt->np.local_ip));
+}
+
+static ssize_t show_remote_ip(struct netconsole_target *nt, char *buf)
+{
+   return sprintf(buf, "%d.%d.%d.%d\n", HIPQUAD(nt->np.remote_ip));
+}
+
+static ssize_t show_local_mac(struct netconsole_target *nt, char *buf)
+{
+   return sprintf(buf, "%02x:%02x:%02x:%02x:%02x:%02x\n",
+  nt->np.local_mac[0], nt->np.local_mac[1],
+  nt->np.local_mac[2], nt->np.local_mac[3],
+  nt->np.local_mac[4], nt->np.local_mac[5]);
+}
+
+static ssize_t show_remote_mac(struct netconsole_target *nt, char *buf)
+{
+   return sprintf(buf, "%02x:%02x:%02x:%02x:%02x:%02x\n",
+  nt->np.remote_mac[0], nt->np.remote_mac[1],
+  nt->np.remote_mac[2], nt->np.remote_mac[3],
+  nt->np.remote_mac[4], nt->np.remote_mac[5]);
+}
+
+static ssize_t store_local_port(struct netconsole_target *nt, const char *buf,
+   size_t count)
+{
+   spin_lock(&target_list_lock);
+   nt->np.local_port = simple_strtol(buf, NULL, 10);
+   spin_unlock(&target_list_lock);
+
+   return count;
+}
+
+static ssize_t store_remote_port(struct netconsole_target *nt, const char *buf,
+   size_t count)
+{
+   spin_lock(&target_list_lock);
+   nt->np.remote_port = simple_strtol(buf, NULL, 10);
+   spin_unlock(&target_list_lock);
+
+   return count;
+}
+
+static ssize_t store_local_ip(struct netconsole_target *nt, const char *buf,
+ size_t count)
+{
+   spin_lock(&target_list_lock);
+   nt->np.local_ip = ntohl(in_aton(buf));
+   spin_unlock(&target_list_lock);
+
+   return count;
+}
+
+s