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

[Satyam Sharma]

Hi,

On 6/13/07, Keiichi KII <k-keiichi@xxxxxxxxxxxxx> wrote:
> From: Keiichi KII <k-keiichi@xxxxxxxxxxxxx>
>
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/