* [PATCH] [ATM]: Add error handling for proc_create fail @ 2008-02-29 7:43 Wang Chen 2008-02-29 12:42 ` chas williams - CONTRACTOR 2008-02-29 18:38 ` David Miller 0 siblings, 2 replies; 8+ messages in thread From: Wang Chen @ 2008-02-29 7:43 UTC (permalink / raw) To: David S. Miller, NETDEV When proc_create() fail, we return -ENOMEM. Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com> --- net/atm/clip.c | 4 ++++ net/atm/lec.c | 4 ++++ 2 files changed, 8 insertions(+), 0 deletions(-) diff --git a/net/atm/clip.c b/net/atm/clip.c index d30167c..57ed448 100644 --- a/net/atm/clip.c +++ b/net/atm/clip.c @@ -963,6 +963,10 @@ static int __init atm_clip_init(void) struct proc_dir_entry *p; p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops); + if (!p) { + printk(KERN_ERR "Unable to initialize /proc/atm/arp\n"); + return -ENOMEM; + } } #endif diff --git a/net/atm/lec.c b/net/atm/lec.c index 0e450d1..532965d 100644 --- a/net/atm/lec.c +++ b/net/atm/lec.c @@ -1250,6 +1250,10 @@ static int __init lane_module_init(void) struct proc_dir_entry *p; p = proc_create("lec", S_IRUGO, atm_proc_root, &lec_seq_fops); + if (!p) { + printk(KERN_ERR "Unable to initialize /proc/atm/lec\n"); + return -ENOMEM; + } #endif register_atm_ioctl(&lane_ioctl_ops); -- WCN ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] [ATM]: Add error handling for proc_create fail 2008-02-29 7:43 [PATCH] [ATM]: Add error handling for proc_create fail Wang Chen @ 2008-02-29 12:42 ` chas williams - CONTRACTOR 2008-02-29 18:38 ` David Miller 1 sibling, 0 replies; 8+ messages in thread From: chas williams - CONTRACTOR @ 2008-02-29 12:42 UTC (permalink / raw) To: Wang Chen; +Cc: NETDEV it should be /proc/net/atm/foo not /proc/atm/foo i think. In message <47C7B79C.2010304@cn.fujitsu.com>,Wang Chen writes: >When proc_create() fail, we return -ENOMEM. > >Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com> >--- > net/atm/clip.c | 4 ++++ > net/atm/lec.c | 4 ++++ > 2 files changed, 8 insertions(+), 0 deletions(-) > >diff --git a/net/atm/clip.c b/net/atm/clip.c >index d30167c..57ed448 100644 >--- a/net/atm/clip.c >+++ b/net/atm/clip.c >@@ -963,6 +963,10 @@ static int __init atm_clip_init(void) > struct proc_dir_entry *p; > > p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops); >+ if (!p) { >+ printk(KERN_ERR "Unable to initialize /proc/atm/arp\n"); >+ return -ENOMEM; >+ } > } > #endif > >diff --git a/net/atm/lec.c b/net/atm/lec.c >index 0e450d1..532965d 100644 >--- a/net/atm/lec.c >+++ b/net/atm/lec.c >@@ -1250,6 +1250,10 @@ static int __init lane_module_init(void) > struct proc_dir_entry *p; > > p = proc_create("lec", S_IRUGO, atm_proc_root, &lec_seq_fops); >+ if (!p) { >+ printk(KERN_ERR "Unable to initialize /proc/atm/lec\n"); >+ return -ENOMEM; >+ } > #endif > > register_atm_ioctl(&lane_ioctl_ops); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [ATM]: Add error handling for proc_create fail 2008-02-29 7:43 [PATCH] [ATM]: Add error handling for proc_create fail Wang Chen 2008-02-29 12:42 ` chas williams - CONTRACTOR @ 2008-02-29 18:38 ` David Miller 2008-03-03 2:44 ` take#2: " Wang Chen 1 sibling, 1 reply; 8+ messages in thread From: David Miller @ 2008-02-29 18:38 UTC (permalink / raw) To: wangchen; +Cc: netdev From: Wang Chen <wangchen@cn.fujitsu.com> Date: Fri, 29 Feb 2008 15:43:24 +0800 > @@ -963,6 +963,10 @@ static int __init atm_clip_init(void) > struct proc_dir_entry *p; > > p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops); > + if (!p) { > + printk(KERN_ERR "Unable to initialize /proc/atm/arp\n"); > + return -ENOMEM; > + } > } > #endif > This is not sufficient. If this fails you need to unregister the ioctl and all of the other things that get registered before this procfs registry runs. ^ permalink raw reply [flat|nested] 8+ messages in thread
* take#2: [PATCH] [ATM]: Add error handling for proc_create fail 2008-02-29 18:38 ` David Miller @ 2008-03-03 2:44 ` Wang Chen 2008-03-03 5:09 ` David Miller 0 siblings, 1 reply; 8+ messages in thread From: Wang Chen @ 2008-03-03 2:44 UTC (permalink / raw) To: David Miller; +Cc: netdev, chas3 David Miller said the following on 2008-3-1 2:38: > This is not sufficient. If this fails you need to unregister > the ioctl and all of the other things that get registered > before this procfs registry runs. > When proc_create() fail, we return -ENOMEM. Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com> --- net/atm/clip.c | 18 ++++++++++-------- net/atm/lec.c | 4 ++++ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/net/atm/clip.c b/net/atm/clip.c index d30167c..66e5a91 100644 --- a/net/atm/clip.c +++ b/net/atm/clip.c @@ -949,6 +949,16 @@ static const struct file_operations arp_seq_fops = { static int __init atm_clip_init(void) { +#ifdef CONFIG_PROC_FS + struct proc_dir_entry *p; + + p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops); + if (!p) { + printk(KERN_ERR "Unable to initialize /proc/net/atm/arp\n"); + return -ENOMEM; + } +#endif + neigh_table_init_no_netlink(&clip_tbl); clip_tbl_hook = &clip_tbl; @@ -958,14 +968,6 @@ static int __init atm_clip_init(void) setup_timer(&idle_timer, idle_timer_check, 0); -#ifdef CONFIG_PROC_FS - { - struct proc_dir_entry *p; - - p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops); - } -#endif - return 0; } diff --git a/net/atm/lec.c b/net/atm/lec.c index 0e450d1..a2efa7f 100644 --- a/net/atm/lec.c +++ b/net/atm/lec.c @@ -1250,6 +1250,10 @@ static int __init lane_module_init(void) struct proc_dir_entry *p; p = proc_create("lec", S_IRUGO, atm_proc_root, &lec_seq_fops); + if (!p) { + printk(KERN_ERR "Unable to initialize /proc/net/atm/lec\n"); + return -ENOMEM; + } #endif register_atm_ioctl(&lane_ioctl_ops); -- WCN ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: take#2: [PATCH] [ATM]: Add error handling for proc_create fail 2008-03-03 2:44 ` take#2: " Wang Chen @ 2008-03-03 5:09 ` David Miller 2008-03-03 8:46 ` take#3: " Wang Chen 0 siblings, 1 reply; 8+ messages in thread From: David Miller @ 2008-03-03 5:09 UTC (permalink / raw) To: wangchen; +Cc: netdev, chas3 From: Wang Chen <wangchen@cn.fujitsu.com> Date: Mon, 03 Mar 2008 10:44:33 +0800 > David Miller said the following on 2008-3-1 2:38: > > This is not sufficient. If this fails you need to unregister > > the ioctl and all of the other things that get registered > > before this procfs registry runs. > > > > When proc_create() fail, we return -ENOMEM. > > Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com> Sigh.... I know you want to avoid all of the error handling unwind code, but you can't do that in this situation. It's not valid to register the ATM clip procfs file without first initializing the ATM clip neighbour table, so moving it up like this isn't going to work. They way you've done it, a user can open the procfs file and access the datastructure before it's initialized properly. Please, simply add the necessary unregister code and leave the procfs initialization where it currently is. I get the sense you really aren't reading this code carefully and you're tossing these patches together as quickly as you can to just get it done. Please don't do that, it wastes both my time and your's. Thank you. ^ permalink raw reply [flat|nested] 8+ messages in thread
* take#3: [PATCH] [ATM]: Add error handling for proc_create fail 2008-03-03 5:09 ` David Miller @ 2008-03-03 8:46 ` Wang Chen 2008-03-04 7:27 ` take#4: " Wang Chen 0 siblings, 1 reply; 8+ messages in thread From: Wang Chen @ 2008-03-03 8:46 UTC (permalink / raw) To: David Miller; +Cc: netdev, chas3 David Miller said the following on 2008-3-3 13:09: > Sigh.... > > I know you want to avoid all of the error handling unwind code, but > you can't do that in this situation. It's not valid to register the > ATM clip procfs file without first initializing the ATM clip neighbour > table, so moving it up like this isn't going to work. > > They way you've done it, a user can open the procfs file and access > the datastructure before it's initialized properly. > > Please, simply add the necessary unregister code and leave the procfs > initialization where it currently is. > > I get the sense you really aren't reading this code carefully and > you're tossing these patches together as quickly as you can to just > get it done. Please don't do that, it wastes both my time and > your's. > No, I didn't read the code very carefully. Sorry for that. However, I don't think it's necessary to put procfs init after ioctl, although it's necessary to do clip procfs init after neighbour tbl init. If i am wrong, please correct me. But I still put clip profs init where it was, because all the unregister can be done easily when I pick the code up to a static function called atm_clip_exit_noproc(). Here is the new patch. --- When proc_create() fails, do some error handling work and return -ENOMEM. Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com> --- net/atm/clip.c | 61 +++++++++++++++++++++++++++++++++----------------------- net/atm/lec.c | 4 +++ 2 files changed, 40 insertions(+), 25 deletions(-) diff --git a/net/atm/clip.c b/net/atm/clip.c index d30167c..24c60ba 100644 --- a/net/atm/clip.c +++ b/net/atm/clip.c @@ -947,34 +947,10 @@ static const struct file_operations arp_seq_fops = { }; #endif -static int __init atm_clip_init(void) -{ - neigh_table_init_no_netlink(&clip_tbl); - - clip_tbl_hook = &clip_tbl; - register_atm_ioctl(&clip_ioctl_ops); - register_netdevice_notifier(&clip_dev_notifier); - register_inetaddr_notifier(&clip_inet_notifier); - - setup_timer(&idle_timer, idle_timer_check, 0); - -#ifdef CONFIG_PROC_FS - { - struct proc_dir_entry *p; - - p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops); - } -#endif - - return 0; -} - -static void __exit atm_clip_exit(void) +static void atm_clip_exit_noproc(void) { struct net_device *dev, *next; - remove_proc_entry("arp", atm_proc_root); - unregister_inetaddr_notifier(&clip_inet_notifier); unregister_netdevice_notifier(&clip_dev_notifier); @@ -1005,6 +981,41 @@ static void __exit atm_clip_exit(void) clip_tbl_hook = NULL; } +static int __init atm_clip_init(void) +{ + neigh_table_init_no_netlink(&clip_tbl); + + clip_tbl_hook = &clip_tbl; + register_atm_ioctl(&clip_ioctl_ops); + register_netdevice_notifier(&clip_dev_notifier); + register_inetaddr_notifier(&clip_inet_notifier); + + setup_timer(&idle_timer, idle_timer_check, 0); + +#ifdef CONFIG_PROC_FS + { + struct proc_dir_entry *p; + + p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops); + if (!p) { + printk(KERN_ERR "Unable to initialize " + "/proc/net/atm/arp\n"); + atm_clip_exit_noproc(); + return -ENOMEM; + } + } +#endif + + return 0; +} + +static void __exit atm_clip_exit(void) +{ + remove_proc_entry("arp", atm_proc_root); + + atm_clip_exit_noproc(); +} + module_init(atm_clip_init); module_exit(atm_clip_exit); MODULE_AUTHOR("Werner Almesberger"); diff --git a/net/atm/lec.c b/net/atm/lec.c index 0e450d1..a2efa7f 100644 --- a/net/atm/lec.c +++ b/net/atm/lec.c @@ -1250,6 +1250,10 @@ static int __init lane_module_init(void) struct proc_dir_entry *p; p = proc_create("lec", S_IRUGO, atm_proc_root, &lec_seq_fops); + if (!p) { + printk(KERN_ERR "Unable to initialize /proc/net/atm/lec\n"); + return -ENOMEM; + } #endif register_atm_ioctl(&lane_ioctl_ops); -- WCN ^ permalink raw reply related [flat|nested] 8+ messages in thread
* take#4: [PATCH] [ATM]: Add error handling for proc_create fail 2008-03-03 8:46 ` take#3: " Wang Chen @ 2008-03-04 7:27 ` Wang Chen 2008-03-24 4:47 ` David Miller 0 siblings, 1 reply; 8+ messages in thread From: Wang Chen @ 2008-03-04 7:27 UTC (permalink / raw) To: David Miller; +Cc: netdev, chas3 Wang Chen said the following on 2008-3-3 16:46: > David Miller said the following on 2008-3-3 13:09: >> Sigh.... >> >> I know you want to avoid all of the error handling unwind code, but >> you can't do that in this situation. It's not valid to register the >> ATM clip procfs file without first initializing the ATM clip neighbour >> table, so moving it up like this isn't going to work. >> >> They way you've done it, a user can open the procfs file and access >> the datastructure before it's initialized properly. >> >> Please, simply add the necessary unregister code and leave the procfs >> initialization where it currently is. >> >> I get the sense you really aren't reading this code carefully and >> you're tossing these patches together as quickly as you can to just >> get it done. Please don't do that, it wastes both my time and >> your's. >> > > No, I didn't read the code very carefully. Sorry for that. > However, I don't think it's necessary to put procfs init after ioctl, > although it's necessary to do clip procfs init after neighbour tbl init. > If i am wrong, please correct me. > > But I still put clip profs init where it was, because all the unregister > can be done easily when I pick the code up to a static function called > atm_clip_exit_noproc(). > > Here is the new patch. > Hi, David, please ignore the previous patch. It's ugly and difficult to understand. Here is a new one. --- When proc_create() fails, do some error handling work and return -ENOMEM. Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com> --- net/atm/clip.c | 19 ++++++++++++++++--- net/atm/lec.c | 4 ++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/net/atm/clip.c b/net/atm/clip.c index d30167c..2ab1e36 100644 --- a/net/atm/clip.c +++ b/net/atm/clip.c @@ -947,6 +947,8 @@ static const struct file_operations arp_seq_fops = { }; #endif +static void atm_clip_exit_noproc(void); + static int __init atm_clip_init(void) { neigh_table_init_no_netlink(&clip_tbl); @@ -963,18 +965,22 @@ static int __init atm_clip_init(void) struct proc_dir_entry *p; p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops); + if (!p) { + printk(KERN_ERR "Unable to initialize " + "/proc/net/atm/arp\n"); + atm_clip_exit_noproc(); + return -ENOMEM; + } } #endif return 0; } -static void __exit atm_clip_exit(void) +static void atm_clip_exit_noproc(void) { struct net_device *dev, *next; - remove_proc_entry("arp", atm_proc_root); - unregister_inetaddr_notifier(&clip_inet_notifier); unregister_netdevice_notifier(&clip_dev_notifier); @@ -1005,6 +1011,13 @@ static void __exit atm_clip_exit(void) clip_tbl_hook = NULL; } +static void __exit atm_clip_exit(void) +{ + remove_proc_entry("arp", atm_proc_root); + + atm_clip_exit_noproc(); +} + module_init(atm_clip_init); module_exit(atm_clip_exit); MODULE_AUTHOR("Werner Almesberger"); diff --git a/net/atm/lec.c b/net/atm/lec.c index 0e450d1..a2efa7f 100644 --- a/net/atm/lec.c +++ b/net/atm/lec.c @@ -1250,6 +1250,10 @@ static int __init lane_module_init(void) struct proc_dir_entry *p; p = proc_create("lec", S_IRUGO, atm_proc_root, &lec_seq_fops); + if (!p) { + printk(KERN_ERR "Unable to initialize /proc/net/atm/lec\n"); + return -ENOMEM; + } #endif register_atm_ioctl(&lane_ioctl_ops); -- WCN ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: take#4: [PATCH] [ATM]: Add error handling for proc_create fail 2008-03-04 7:27 ` take#4: " Wang Chen @ 2008-03-24 4:47 ` David Miller 0 siblings, 0 replies; 8+ messages in thread From: David Miller @ 2008-03-24 4:47 UTC (permalink / raw) To: wangchen; +Cc: netdev, chas3 From: Wang Chen <wangchen@cn.fujitsu.com> Date: Tue, 04 Mar 2008 15:27:41 +0800 > When proc_create() fails, do some error handling work and return -ENOMEM. > > Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com> Applied, thanks a lot. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2008-03-24 4:47 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-02-29 7:43 [PATCH] [ATM]: Add error handling for proc_create fail Wang Chen 2008-02-29 12:42 ` chas williams - CONTRACTOR 2008-02-29 18:38 ` David Miller 2008-03-03 2:44 ` take#2: " Wang Chen 2008-03-03 5:09 ` David Miller 2008-03-03 8:46 ` take#3: " Wang Chen 2008-03-04 7:27 ` take#4: " Wang Chen 2008-03-24 4:47 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).