Hi,
On 6/13/07, Keiichi KII <[EMAIL PROTECTED]> wrote:
[...]
+static DECLARE_MUTEX(netdev_change_sem);
The preferred style these days is to use a DEFINE_MUTEX
(and the struct mutex primitives) for such locks that are used
as binary semaphores.
BTW, a comment here to note what this lock protects is required.
[ You don't really need to give a comment for the target_list_lock
because it's defined just below the "target_list". It's not equally obvious
at first glance what is protected by the netdev_change_sem, however. ]
+ down(&netdev_change_sem);
+ up(&netdev_change_sem);
So these become mutex_lock and mutex_unlock.
+static int netconsole_event(struct notifier_block *this, unsigned long event,
+ void *ptr)
+{
+ int error = 0;
+ unsigned long flags;
+ char *old_link_name = NULL, *new_link_name = NULL;
+ struct netconsole_target *nt, *tmp;
+ struct net_device *dev = ptr;
+ LIST_HEAD(modify_target_list);
+
+ if (event == NETDEV_CHANGENAME) {
+ spin_lock_irqsave(&target_list_lock, flags);
+ list_for_each_entry_safe(nt, tmp, &target_list, list)
+ if (nt->np.dev == dev)
+ list_move(&nt->list, &modify_target_list);
+ spin_unlock_irqrestore(&target_list_lock, flags);
+
+ down(&netdev_change_sem);
+ list_for_each_entry(nt, &modify_target_list, list) {
+ new_link_name = make_netdev_class_name(dev->name);
+ old_link_name = make_netdev_class_name(nt->np.dev_name);
+ if (!new_link_name || !old_link_name) {
+ printk(KERN_ERR "netconsole: "
+ "make_netdev_class_name() failed!\n");
+ kfree(new_link_name);
+ kfree(old_link_name);
+ continue;
Sorry, but we're not covering from the error condition fully here. Note
that later you merge the temporary modify_target_list entirely back
into the target_list ... which would still contain these erroneous
nodes. A full cleanup (kobject_unregister the entry, and then list_del
from modify_target_list) is required here, before continuing.
+ }
+ sysfs_remove_link(&nt->obj, old_link_name);
+ error = sysfs_create_link(&nt->obj,
+ &nt->np.dev->dev.kobj,
+ new_link_name);
+ if (error)
+ printk(KERN_ERR "can't create link: %s\n",
+ new_link_name);
Same here, a fuller cleanup is required to recover from the error
condition here, then kfree()'s and then stick a "continue;" here, else ...
+ strcpy(nt->np.dev_name, dev->name);
... you'll have move this up.
+ kfree(new_link_name);
+ kfree(old_link_name);
+ }
+ up(&netdev_change_sem);
+
+ spin_lock_irqsave(&target_list_lock, flags);
+ list_for_each_entry_safe(nt, tmp, &modify_target_list, list)
+ list_move(&nt->list, &target_list);
+ spin_unlock_irqrestore(&target_list_lock, flags);
+ }
+
+ return NOTIFY_DONE;
+}
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/
