dm-lc.c: Audit return values of functions invoked in module init routine
PATCH 1/1 dm-lc.c: Audit return values of functions invoked in module init routine
Add missing checks for values returned by various functions for graceful exit in the event of failure.
--- Driver/dm-lc.c | 60 +++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 17 deletions(-)
Driver/dm-lc.c +43 -17
@@ -2959,14 +2959,7 @@ static struct target_type lc_mgr_target = {static int __init lc_module_init (void ) { -int r; - - safe_io_wq =alloc_workqueue ("safeiowq" , -WQ_NON_REENTRANT |WQ_MEM_RECLAIM ,0 ); -if (!safe_io_wq) -return -ENOMEM ; - - lc_io_client =dm_io_client_create (); +int r = -ENOMEM ; r =dm_register_target (&lc_target);if (r <0 ) {
@@ -2980,15 +2973,6 @@ static int __init lc_module_init(void)return r; } - cache_id_ptr =0 ; - -size_t i; -for (i =0 ; i <LC_NR_SLOTS ; i++) - lc_devices[i] =NULL ; - -for (i =0 ; i <LC_NR_SLOTS ; i++) - lc_caches[i] =NULL ; -/* * /sys/module/dm_lc/devices * /caches
@@ -2996,10 +2980,52 @@ static int __init lc_module_init(void)struct module *mod =THIS_MODULE ;struct kobject *lc_kobj = &(mod->mkobj.kobj); + devices_kobj =kobject_create_and_add ("devices" , lc_kobj); +if (!devices_kobj) + goto fail_kobj_devices; + caches_kobj =kobject_create_and_add ("caches" , lc_kobj); +if (!caches_kobj) + goto fail_kobj_caches; + + safe_io_wq =alloc_workqueue ("safeiowq" , +WQ_NON_REENTRANT |WQ_MEM_RECLAIM ,0 ); +if (!safe_io_wq) { +DMERR ("failed to create workqueue safeiowq" ); + goto fail_wq; + } + + lc_io_client =dm_io_client_create (); +if (IS_ERR (lc_io_client)) { + r =PTR_ERR (lc_io_client); + goto fail_io_client; + } + + cache_id_ptr =0 ; + +size_t i; +for (i =0 ; i <LC_NR_SLOTS ; i++) + lc_devices[i] =NULL ; + +for (i =0 ; i <LC_NR_SLOTS ; i++) + lc_caches[i] =NULL ;return 0 ; + +fail_io_client: +destroy_workqueue (safe_io_wq); + +fail_wq: +kobject_put (caches_kobj); + +fail_kobj_caches: +kobject_put (devices_kobj); + +fail_kobj_devices: +dm_unregister_target (&lc_mgr_target); +dm_unregister_target (&lc_target); +return ERR_PTR (r); } static void __exit lc_module_exit(void)
Signature · 1 line
1.8.3.1
Thanks for your patch, Kumar.
First of all, the code got clean
by reordering the initialization calls. I fully agree with your patch at this point. Thanks for your work.
But, I think there are a mistake to fix before applying the patch.
What if registering lc_mgr_target failed
after registering lc_target succeeded?
I think we should unregister the lc_target in this case. Am I wrong?
Looking for some code samples,dm-snap.c and dm-thin.c have more than two targets to register like dm-lc does.
Below is a code fragment from dm-snap.c . It treats each failure in registering targets independently with different labels.
// dm-snap.cbad_exception_cache:
exit_origin_hash();
bad_origin_hash:
dm_unregister_target(&merge_target);
bad_register_merge_target:
dm_unregister_target(&origin_target);
bad_register_origin_target:
dm_unregister_target(&snapshot_target);
bad_register_snapshot_target:
dm_exception_store_exit();
return r;
}Could you please fix this problem
and resend the patch again?
In addition, please rename the labels from fail_ to bad_ .
The prefix bad_ seems to be the
code convention in device-mapper.
Akira
Quoted 98 lines · Kumar Amit Mehta
Add missing checks for values returned by various functions for graceful exit in the event of failure.
Signed-off-by Kumar Amit Mehta <[email protected]>--- Driver/dm-lc.c | 60 +++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 17 deletions(-)Driver/dm-lc.c +43
-17@@ -2959,14 +2959,7 @@ static struct target_type lc_mgr_target = {static int __init lc_module_init (void ) { -int r; - - safe_io_wq =alloc_workqueue ("safeiowq" , -WQ_NON_REENTRANT |WQ_MEM_RECLAIM ,0 ); -if (!safe_io_wq) -return -ENOMEM ; - - lc_io_client =dm_io_client_create (); +int r = -ENOMEM ; r =dm_register_target (&lc_target);if (r <0 ) {@@ -2980,15 +2973,6 @@ static int __init lc_module_init(void)return r; } - cache_id_ptr =0 ; - -size_t i; -for (i =0 ; i <LC_NR_SLOTS ; i++) - lc_devices[i] =NULL ; - -for (i =0 ; i <LC_NR_SLOTS ; i++) - lc_caches[i] =NULL ; -/* * /sys/module/dm_lc/devices * /caches @@ -2996,10 +2980,52 @@ static int __init lc_module_init(void)struct module *mod =THIS_MODULE ;struct kobject *lc_kobj = &(mod->mkobj.kobj); + devices_kobj =kobject_create_and_add ("devices" , lc_kobj); +if (!devices_kobj) + goto fail_kobj_devices; + caches_kobj =kobject_create_and_add ("caches" , lc_kobj); +if (!caches_kobj) + goto fail_kobj_caches; + + safe_io_wq =alloc_workqueue ("safeiowq" , +WQ_NON_REENTRANT |WQ_MEM_RECLAIM ,0 ); +if (!safe_io_wq) { +DMERR ("failed to create workqueue safeiowq" ); + goto fail_wq; + } + + lc_io_client =dm_io_client_create (); +if (IS_ERR (lc_io_client)) { + r =PTR_ERR (lc_io_client); + goto fail_io_client; + } + + cache_id_ptr =0 ; + +size_t i; +for (i =0 ; i <LC_NR_SLOTS ; i++) + lc_devices[i] =NULL ; + +for (i =0 ; i <LC_NR_SLOTS ; i++) + lc_caches[i] =NULL ;return 0 ; + +fail_io_client: +destroy_workqueue (safe_io_wq); + +fail_wq: +kobject_put (caches_kobj); + +fail_kobj_caches: +kobject_put (devices_kobj); + +fail_kobj_devices: +dm_unregister_target (&lc_mgr_target); +dm_unregister_target (&lc_target); +return ERR_PTR (r); } static void __exit lc_module_exit(void)
Quoted 4 lines · Akira Hayakawa
What if registering
lc_mgr_targetfailed
after registeringlc_targetsucceeded?
I think we should unregister thelc_targetin this case. Am I wrong?
You are correct. I'll make the changes.
Quoted 10 lines
Looking for some code samples,
dm-snap.canddm-thin.chave more than two targets to register like dm-lc does.Below is a code fragment from
dm-snap.c. It treats each failure in registering targets independently with different labels.//
dm-snap.cbad_exception_cache:
Quoted 11 lines
exit_origin_hash();
bad_origin_hash:
dm_unregister_target(&merge_target);
bad_register_merge_target:
dm_unregister_target(&origin_target);
bad_register_origin_target:
dm_unregister_target(&snapshot_target);
bad_register_snapshot_target:
dm_exception_store_exit();
return r;Thank you for your feedback and the pointers, I'll resend the patch.
!!amit