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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 6/13/07, Keiichi KII <[email protected]> wrote:
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
   | |--- local_ip    [rw-r--r--]  source IP to use, writable
   | |--- local_mac   [r--r--r--]  source MAC address
   | |--- local_port  [rw-r--r--]  source port number for UDP packets, writable
   | |--- remote_ip   [rw-r--r--]  port number for logging agent, writable
   | |--- remote_mac  [rw-r--r--]  MAC address for logging agent, writable
   | ---- remote_port [rw-r--r--]  IP address for logging agent, writable
   |--- port2/
   |--- port3/

[...]
+static int miscdev_configured;

Is this really required? We just return with error if misc_register()
fails during module init time itself, so it's not really useful ever, is it?

+static ssize_t show_target_attr(struct kobject *kobj,
[...]
+       if (na->show)
+               return na->show(nt, buffer);
+       else
+               return -EIO;

and

+static ssize_t store_target_attr(struct kobject *kobj,
[...]
+       if (na->store)
+               return na->store(nt, buffer, count);
+       else
+               return -EIO;

EIO isn't quite appropriate for the above two cases. EPERM?

+static int setup_target_sysfs(struct netconsole_target *nt)
+{
+       kobject_set_name(&nt->obj, "port%d", nt->id);

Ah, I see, so this is where we use the ->id member.

@@ -192,7 +386,9 @@ static void cleanup_netconsole(void)

        unregister_console(&netconsole);
        list_for_each_entry_safe(nt, tmp, &target_list, list)
-               remove_target(nt);
+               kobject_unregister(&nt->obj);
+       if (miscdev_configured)
+               misc_deregister(&netconsole_miscdev);

Yeah, I suspect we can do away with miscdev_configured here.
We reach this code only if it is known to be true.

+       if (misc_register(&netconsole_miscdev)) {
+               printk(KERN_ERR
+                      "netconsole: failed to register misc device for "
+                      "name = %s, id = %d\n",
+                      netconsole_miscdev.name, netconsole_miscdev.minor);
+               return -EIO;
+       } else
+               miscdev_configured = 1;
+

Please prefer:

err = misc_register(...);
if (err)
       ...
       return err;

That way we avoid the weird EIO and maintain the error code
returned by misc_register().

Satyam
-
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/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux