[PATCH 0/2] Declare functions and structures as static.

[PATCH 0/2] Declare functions and structures as static.

[Shraddha Barke]

These patches declare the functions and structures which are not
used anywhere else as static.

Shraddha Barke (2):
  Staging: lustre: ptlrpc: Make functions static
  Staging: lustre: obdclass: Declare structure as static.

 drivers/staging/lustre/lustre/obdclass/lu_object.c  | 2 +-
 drivers/staging/lustre/lustre/ptlrpc/pack_generic.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.1.4

[PATCH 1/2] Staging: lustre: ptlrpc: Make functions static

[Shraddha Barke]

Since these functions are not called anywhere apart from this particular
file, declare them as static.

Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
---
 drivers/staging/lustre/lustre/ptlrpc/pack_generic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c b/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c
index d2fc627..0d32147 100644
--- a/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c
+++ b/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c
@@ -1929,7 +1929,7 @@ void lustre_swab_fid2path(struct getinfo_fid2path *gf)
 }
 EXPORT_SYMBOL(lustre_swab_fid2path);
 
-void lustre_swab_fiemap_extent(struct ll_fiemap_extent *fm_extent)
+static void lustre_swab_fiemap_extent(struct ll_fiemap_extent *fm_extent)
 {
 	__swab64s(&fm_extent->fe_logical);
 	__swab64s(&fm_extent->fe_physical);
@@ -2405,7 +2405,7 @@ void lustre_swab_hsm_state_set(struct hsm_state_set *hss)
 }
 EXPORT_SYMBOL(lustre_swab_hsm_state_set);
 
-void lustre_swab_hsm_extent(struct hsm_extent *extent)
+static void lustre_swab_hsm_extent(struct hsm_extent *extent)
 {
 	__swab64s(&extent->offset);
 	__swab64s(&extent->length);
-- 
2.1.4

Re: [Outreachy kernel] [PATCH 1/2] Staging: lustre: ptlrpc: Make functions static

[Arnd Bergmann]

On Thursday 01 October 2015 15:07:57 Shraddha Barke wrote:
> Since these functions are not called anywhere apart from this particular
> file, declare them as static.
> 
> Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
> 

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Note that there are hundreds more symbols in lustre that should be made static.
Most of them are harder to find because they have a declaration in a header file
despite being only used in one place.

Coincidentally I've recently checked this for a subset of lustre while removing
completely unused functions. Have a look at the list below to see if you can
do more of those.

	Arnd

find build/tmp/drivers/staging/lustre/lustre/{ptlrpc,obdclass}/ -name \*.o | \
	grep -v built-in | xargs nm | grep -w [A-TV-Z] | grep -v __ksymtab | 
	grep -v __crc | cut -f 3 -d \  | sort -u > defs
find build/tmp/drivers/staging/lustre/ -name \*.o | grep -v built-in | \
	xargs nm | grep -w U | cut -f 11- -d\  | sort -u > refs
diff -u defs refs | grep ^-  | cut -f 2 -d-

__ptlrpc_free_bulk
cache_stats_init
cache_stats_print
capa_add
capa_cpy
capa_decrypt_id
capa_encrypt_id
capa_hmac
capa_lookup
cl_env_peek
cl_io_cancel
cl_io_rw_advance
cl_lock_enclosure
cl_lock_extransit
cl_lock_intransit
cl_lock_mutex_try
cl_lock_state_wait
cl_page_list_assume
cl_page_list_del
cl_page_list_discard
cl_page_list_fini
cl_sync_io_init
cl_sync_io_wait
class_add_conn
class_add_profile
class_attach
class_cleanup
class_config_dump_handler
class_config_parse_rec
class_del_conn
class_detach
class_get_type
class_match_param
class_resolve_dev_name
class_search_type
class_setup
cleanup_capa_hash
deuuidify
do_lcfg
dt_acct_features
dt_declare_version_set
dt_device_fini
dt_device_init
dt_directory_features
dt_find_or_create
dt_global_fini
dt_global_init
dt_index_read
dt_index_walk
dt_key
dt_lfsck_features
dt_locate_at
dt_lookup_dir
dt_mode_to_dft
dt_object_fini
dt_object_init
dt_otable_features
dt_path_parser
dt_quota_glb_features
dt_quota_slv_features
dt_read
dt_record_read
dt_record_write
dt_store_open
dt_store_resolve
dt_try_as_dir
dt_txn_callback_add
dt_txn_callback_del
dt_txn_hook_commit
dt_txn_hook_start
dt_txn_hook_stop
dt_version_get
dt_version_set
dump_exports
dump_obdo
iattr_from_obdo
init_capa_hash
interval_expand
interval_find
interval_is_overlapped
interval_iterate
interval_iterate_reverse
interval_search
ldlm_add_bl_work_item
ldlm_add_cp_work_item
ldlm_blocking_ast
ldlm_blocking_ast_nocheck
ldlm_cancel_locks_for_export
ldlm_cancel_locks_for_export_cb
ldlm_cancel_lru_local
ldlm_cli_cancel_req
ldlm_cli_convert
ldlm_cli_enqueue_local
ldlm_cli_inactive_namespace_list
ldlm_convert_policy_to_wire
ldlm_destroy_export
ldlm_dump_granted_max
ldlm_enqueue_pack
ldlm_errno2error
ldlm_expired_completion_wait
ldlm_flock_blocking_ast
ldlm_get_enq_timeout
ldlm_glimpse_ast
ldlm_init_export
ldlm_kobj
ldlm_lock_add_to_lru
ldlm_lock_add_to_lru_nolock
ldlm_lock_destroy_internal
ldlm_lock_downgrade
ldlm_lock_fail_match
ldlm_lock_touch_in_lru
ldlm_namespace_foreach
ldlm_namespace_free
ldlm_namespace_get_return
ldlm_namespace_register
ldlm_namespace_sysfs_register
ldlm_namespace_sysfs_unregister
ldlm_namespace_unregister
ldlm_ns_fid_hash_ops
ldlm_ns_hash_defs
ldlm_ns_hash_ops
ldlm_pool_get_clv
ldlm_pool_get_limit
ldlm_pool_recalc
ldlm_pool_set_limit
ldlm_pool_set_slv
ldlm_pool_setup
ldlm_pool_shrink
ldlm_pools_recalc
ldlm_process_flock_lock
ldlm_refresh_waiting_lock
ldlm_register_intent
ldlm_reprocess_all_ns
ldlm_resource_foreach
ldlm_resource_insert_lock_after
ldlm_resource_putref_locked
ldlm_svc_kset
ldlm_typename
ldlm_work_gl_ast_lock
llog_cat_id2handle
llog_cat_process_or_fork
llog_thread_key
lock_handle_ops
lprocfs_dt_rd_blksize
lprocfs_dt_rd_filesfree
lprocfs_dt_rd_filestotal
lprocfs_dt_rd_kbytesavail
lprocfs_dt_rd_kbytesfree
lprocfs_dt_rd_kbytestotal
lprocfs_stats_seq_fops
lprocfs_write_frac_u64_helper
lu_context_key_quiesce
lu_context_key_revive
lu_context_refill
lu_dev_add_linkage
lu_device_put
lu_global_key
lu_object_find
lu_site_hash_ops
lustre_attr_show
lustre_attr_store
lustre_fill_super
lustre_fs_type
lustre_init_lsi
lustre_mount
lustre_pack_reply_v2
lustre_profile_list
lustre_put_lsi
lustre_start_mgc
lustre_start_simple
lustre_swab_ldlm_lock_desc
lustre_swab_ldlm_policy_data
lustre_swab_ldlm_res_id
lustre_swab_ldlm_resource_desc
lustre_swab_ll_fid
lustre_swab_llog_id
lustre_swab_obdo
mgc_start_lock
nrs_policy_get_info_locked
obd_export_nid2str
obd_init_checks
obd_zombie_impexp_cull
ptl_get_pid
ptlrpc_interrupted_set
ptlrpc_new_bulk
ptlrpc_next_xid
ptlrpc_nrs_policy_register
ptlrpc_pinger_remove_timeouts
ptlrpc_register_bulk
ptlrpc_retain_replayable_request
ptlrpc_server_drop_request
ptlrpc_start_thread
ptlrpc_start_threads
ptlrpc_stop_all_threads
ptlrpc_unregister_reply
ptlrpcd_free
ptlrpcd_start
ptlrpcd_stop
req_capsule_field_present
req_capsule_init_area
server_name2fsname
server_name2index
sptlrpc_cli_wrap_bulk
sptlrpc_parse_rule
sptlrpc_req_replace_dead_ctx
sptlrpc_rule_set_choose
sptlrpc_rule_set_expand
sptlrpc_rule_set_free
sptlrpc_rule_set_merge
sptlrpc_sec_get
sptlrpc_secflags2str
sptlrpc_svc_install_rvs_ctx

Re: [Outreachy kernel] [PATCH 1/2] Staging: lustre: ptlrpc: Make functions static

[Shraddha Barke]

On Thu, 1 Oct 2015, Arnd Bergmann wrote:

> On Thursday 01 October 2015 15:07:57 Shraddha Barke wrote:
>> Since these functions are not called anywhere apart from this particular
>> file, declare them as static.
>>
>> Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
>>
>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>
> Note that there are hundreds more symbols in lustre that should be made static.
> Most of them are harder to find because they have a declaration in a header file
> despite being only used in one place.
>
> Coincidentally I've recently checked this for a subset of lustre while removing
> completely unused functions. Have a look at the list below to see if you can
> do more of those.

Yes I'll work on these.

Thanks :)
>
> 	Arnd
>
> find build/tmp/drivers/staging/lustre/lustre/{ptlrpc,obdclass}/ -name \*.o | \
> 	grep -v built-in | xargs nm | grep -w [A-TV-Z] | grep -v __ksymtab |
> 	grep -v __crc | cut -f 3 -d \  | sort -u > defs
> find build/tmp/drivers/staging/lustre/ -name \*.o | grep -v built-in | \
> 	xargs nm | grep -w U | cut -f 11- -d\  | sort -u > refs
> diff -u defs refs | grep ^-  | cut -f 2 -d-
>
> __ptlrpc_free_bulk
> cache_stats_init
> cache_stats_print
> capa_add
> capa_cpy
> capa_decrypt_id
> capa_encrypt_id
> capa_hmac
> capa_lookup
> cl_env_peek
> cl_io_cancel
> cl_io_rw_advance
> cl_lock_enclosure
> cl_lock_extransit
> cl_lock_intransit
> cl_lock_mutex_try
> cl_lock_state_wait
> cl_page_list_assume
> cl_page_list_del
> cl_page_list_discard
> cl_page_list_fini
> cl_sync_io_init
> cl_sync_io_wait
> class_add_conn
> class_add_profile
> class_attach
> class_cleanup
> class_config_dump_handler
> class_config_parse_rec
> class_del_conn
> class_detach
> class_get_type
> class_match_param
> class_resolve_dev_name
> class_search_type
> class_setup
> cleanup_capa_hash
> deuuidify
> do_lcfg
> dt_acct_features
> dt_declare_version_set
> dt_device_fini
> dt_device_init
> dt_directory_features
> dt_find_or_create
> dt_global_fini
> dt_global_init
> dt_index_read
> dt_index_walk
> dt_key
> dt_lfsck_features
> dt_locate_at
> dt_lookup_dir
> dt_mode_to_dft
> dt_object_fini
> dt_object_init
> dt_otable_features
> dt_path_parser
> dt_quota_glb_features
> dt_quota_slv_features
> dt_read
> dt_record_read
> dt_record_write
> dt_store_open
> dt_store_resolve
> dt_try_as_dir
> dt_txn_callback_add
> dt_txn_callback_del
> dt_txn_hook_commit
> dt_txn_hook_start
> dt_txn_hook_stop
> dt_version_get
> dt_version_set
> dump_exports
> dump_obdo
> iattr_from_obdo
> init_capa_hash
> interval_expand
> interval_find
> interval_is_overlapped
> interval_iterate
> interval_iterate_reverse
> interval_search
> ldlm_add_bl_work_item
> ldlm_add_cp_work_item
> ldlm_blocking_ast
> ldlm_blocking_ast_nocheck
> ldlm_cancel_locks_for_export
> ldlm_cancel_locks_for_export_cb
> ldlm_cancel_lru_local
> ldlm_cli_cancel_req
> ldlm_cli_convert
> ldlm_cli_enqueue_local
> ldlm_cli_inactive_namespace_list
> ldlm_convert_policy_to_wire
> ldlm_destroy_export
> ldlm_dump_granted_max
> ldlm_enqueue_pack
> ldlm_errno2error
> ldlm_expired_completion_wait
> ldlm_flock_blocking_ast
> ldlm_get_enq_timeout
> ldlm_glimpse_ast
> ldlm_init_export
> ldlm_kobj
> ldlm_lock_add_to_lru
> ldlm_lock_add_to_lru_nolock
> ldlm_lock_destroy_internal
> ldlm_lock_downgrade
> ldlm_lock_fail_match
> ldlm_lock_touch_in_lru
> ldlm_namespace_foreach
> ldlm_namespace_free
> ldlm_namespace_get_return
> ldlm_namespace_register
> ldlm_namespace_sysfs_register
> ldlm_namespace_sysfs_unregister
> ldlm_namespace_unregister
> ldlm_ns_fid_hash_ops
> ldlm_ns_hash_defs
> ldlm_ns_hash_ops
> ldlm_pool_get_clv
> ldlm_pool_get_limit
> ldlm_pool_recalc
> ldlm_pool_set_limit
> ldlm_pool_set_slv
> ldlm_pool_setup
> ldlm_pool_shrink
> ldlm_pools_recalc
> ldlm_process_flock_lock
> ldlm_refresh_waiting_lock
> ldlm_register_intent
> ldlm_reprocess_all_ns
> ldlm_resource_foreach
> ldlm_resource_insert_lock_after
> ldlm_resource_putref_locked
> ldlm_svc_kset
> ldlm_typename
> ldlm_work_gl_ast_lock
> llog_cat_id2handle
> llog_cat_process_or_fork
> llog_thread_key
> lock_handle_ops
> lprocfs_dt_rd_blksize
> lprocfs_dt_rd_filesfree
> lprocfs_dt_rd_filestotal
> lprocfs_dt_rd_kbytesavail
> lprocfs_dt_rd_kbytesfree
> lprocfs_dt_rd_kbytestotal
> lprocfs_stats_seq_fops
> lprocfs_write_frac_u64_helper
> lu_context_key_quiesce
> lu_context_key_revive
> lu_context_refill
> lu_dev_add_linkage
> lu_device_put
> lu_global_key
> lu_object_find
> lu_site_hash_ops
> lustre_attr_show
> lustre_attr_store
> lustre_fill_super
> lustre_fs_type
> lustre_init_lsi
> lustre_mount
> lustre_pack_reply_v2
> lustre_profile_list
> lustre_put_lsi
> lustre_start_mgc
> lustre_start_simple
> lustre_swab_ldlm_lock_desc
> lustre_swab_ldlm_policy_data
> lustre_swab_ldlm_res_id
> lustre_swab_ldlm_resource_desc
> lustre_swab_ll_fid
> lustre_swab_llog_id
> lustre_swab_obdo
> mgc_start_lock
> nrs_policy_get_info_locked
> obd_export_nid2str
> obd_init_checks
> obd_zombie_impexp_cull
> ptl_get_pid
> ptlrpc_interrupted_set
> ptlrpc_new_bulk
> ptlrpc_next_xid
> ptlrpc_nrs_policy_register
> ptlrpc_pinger_remove_timeouts
> ptlrpc_register_bulk
> ptlrpc_retain_replayable_request
> ptlrpc_server_drop_request
> ptlrpc_start_thread
> ptlrpc_start_threads
> ptlrpc_stop_all_threads
> ptlrpc_unregister_reply
> ptlrpcd_free
> ptlrpcd_start
> ptlrpcd_stop
> req_capsule_field_present
> req_capsule_init_area
> server_name2fsname
> server_name2index
> sptlrpc_cli_wrap_bulk
> sptlrpc_parse_rule
> sptlrpc_req_replace_dead_ctx
> sptlrpc_rule_set_choose
> sptlrpc_rule_set_expand
> sptlrpc_rule_set_free
> sptlrpc_rule_set_merge
> sptlrpc_sec_get
> sptlrpc_secflags2str
> sptlrpc_svc_install_rvs_ctx
>
>

Re: [Outreachy kernel] [PATCH 1/2] Staging: lustre: ptlrpc: Make functions static

[Julia Lawall]

On Thu, 1 Oct 2015, Shraddha Barke wrote:

>
>
> On Thu, 1 Oct 2015, Arnd Bergmann wrote:
>
> > On Thursday 01 October 2015 15:07:57 Shraddha Barke wrote:
> > > Since these functions are not called anywhere apart from this particular
> > > file, declare them as static.
> > >
> > > Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
> > >
> >
> > Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> >
> > Note that there are hundreds more symbols in lustre that should be made
> > static.
> > Most of them are harder to find because they have a declaration in a header
> > file
> > despite being only used in one place.

Why does it make it hard to find?  One can make a spearate check in .c
files.

I would think though that the clean up could be a little bit tedious.  I
guess that when the function is static, it shouldn't be declared in the
header file any more?  At least not if the header file is shared
between multipe files.  But it still may need to be declared somewhere, if
it is referenced before it is declared in the one file where it is used.

The question then is how to divide up the patches.  Should one just
address all of the functions that have this property in a single file?  Or
should one only work on a couple of functions at a time that seem related
to each other?

Another issue in Lustre is that there are a some EXPORT_SYMBOLs on
functyions that are only used in one file.  Should they all be removed, or
is it like the case of android where there is code not yet in the kernel
that may rely on them?

julia

> > Coincidentally I've recently checked this for a subset of lustre while
> > removing
> > completely unused functions. Have a look at the list below to see if you can
> > do more of those.
>
> Yes I'll work on these.
>
> Thanks :)
> >
> > 	Arnd
> >
> > find build/tmp/drivers/staging/lustre/lustre/{ptlrpc,obdclass}/ -name \*.o |
> > \
> > 	grep -v built-in | xargs nm | grep -w [A-TV-Z] | grep -v __ksymtab |
> > 	grep -v __crc | cut -f 3 -d \  | sort -u > defs
> > find build/tmp/drivers/staging/lustre/ -name \*.o | grep -v built-in | \
> > 	xargs nm | grep -w U | cut -f 11- -d\  | sort -u > refs
> > diff -u defs refs | grep ^-  | cut -f 2 -d-
> >
> > __ptlrpc_free_bulk
> > cache_stats_init
> > cache_stats_print
> > capa_add
> > capa_cpy
> > capa_decrypt_id
> > capa_encrypt_id
> > capa_hmac
> > capa_lookup
> > cl_env_peek
> > cl_io_cancel
> > cl_io_rw_advance
> > cl_lock_enclosure
> > cl_lock_extransit
> > cl_lock_intransit
> > cl_lock_mutex_try
> > cl_lock_state_wait
> > cl_page_list_assume
> > cl_page_list_del
> > cl_page_list_discard
> > cl_page_list_fini
> > cl_sync_io_init
> > cl_sync_io_wait
> > class_add_conn
> > class_add_profile
> > class_attach
> > class_cleanup
> > class_config_dump_handler
> > class_config_parse_rec
> > class_del_conn
> > class_detach
> > class_get_type
> > class_match_param
> > class_resolve_dev_name
> > class_search_type
> > class_setup
> > cleanup_capa_hash
> > deuuidify
> > do_lcfg
> > dt_acct_features
> > dt_declare_version_set
> > dt_device_fini
> > dt_device_init
> > dt_directory_features
> > dt_find_or_create
> > dt_global_fini
> > dt_global_init
> > dt_index_read
> > dt_index_walk
> > dt_key
> > dt_lfsck_features
> > dt_locate_at
> > dt_lookup_dir
> > dt_mode_to_dft
> > dt_object_fini
> > dt_object_init
> > dt_otable_features
> > dt_path_parser
> > dt_quota_glb_features
> > dt_quota_slv_features
> > dt_read
> > dt_record_read
> > dt_record_write
> > dt_store_open
> > dt_store_resolve
> > dt_try_as_dir
> > dt_txn_callback_add
> > dt_txn_callback_del
> > dt_txn_hook_commit
> > dt_txn_hook_start
> > dt_txn_hook_stop
> > dt_version_get
> > dt_version_set
> > dump_exports
> > dump_obdo
> > iattr_from_obdo
> > init_capa_hash
> > interval_expand
> > interval_find
> > interval_is_overlapped
> > interval_iterate
> > interval_iterate_reverse
> > interval_search
> > ldlm_add_bl_work_item
> > ldlm_add_cp_work_item
> > ldlm_blocking_ast
> > ldlm_blocking_ast_nocheck
> > ldlm_cancel_locks_for_export
> > ldlm_cancel_locks_for_export_cb
> > ldlm_cancel_lru_local
> > ldlm_cli_cancel_req
> > ldlm_cli_convert
> > ldlm_cli_enqueue_local
> > ldlm_cli_inactive_namespace_list
> > ldlm_convert_policy_to_wire
> > ldlm_destroy_export
> > ldlm_dump_granted_max
> > ldlm_enqueue_pack
> > ldlm_errno2error
> > ldlm_expired_completion_wait
> > ldlm_flock_blocking_ast
> > ldlm_get_enq_timeout
> > ldlm_glimpse_ast
> > ldlm_init_export
> > ldlm_kobj
> > ldlm_lock_add_to_lru
> > ldlm_lock_add_to_lru_nolock
> > ldlm_lock_destroy_internal
> > ldlm_lock_downgrade
> > ldlm_lock_fail_match
> > ldlm_lock_touch_in_lru
> > ldlm_namespace_foreach
> > ldlm_namespace_free
> > ldlm_namespace_get_return
> > ldlm_namespace_register
> > ldlm_namespace_sysfs_register
> > ldlm_namespace_sysfs_unregister
> > ldlm_namespace_unregister
> > ldlm_ns_fid_hash_ops
> > ldlm_ns_hash_defs
> > ldlm_ns_hash_ops
> > ldlm_pool_get_clv
> > ldlm_pool_get_limit
> > ldlm_pool_recalc
> > ldlm_pool_set_limit
> > ldlm_pool_set_slv
> > ldlm_pool_setup
> > ldlm_pool_shrink
> > ldlm_pools_recalc
> > ldlm_process_flock_lock
> > ldlm_refresh_waiting_lock
> > ldlm_register_intent
> > ldlm_reprocess_all_ns
> > ldlm_resource_foreach
> > ldlm_resource_insert_lock_after
> > ldlm_resource_putref_locked
> > ldlm_svc_kset
> > ldlm_typename
> > ldlm_work_gl_ast_lock
> > llog_cat_id2handle
> > llog_cat_process_or_fork
> > llog_thread_key
> > lock_handle_ops
> > lprocfs_dt_rd_blksize
> > lprocfs_dt_rd_filesfree
> > lprocfs_dt_rd_filestotal
> > lprocfs_dt_rd_kbytesavail
> > lprocfs_dt_rd_kbytesfree
> > lprocfs_dt_rd_kbytestotal
> > lprocfs_stats_seq_fops
> > lprocfs_write_frac_u64_helper
> > lu_context_key_quiesce
> > lu_context_key_revive
> > lu_context_refill
> > lu_dev_add_linkage
> > lu_device_put
> > lu_global_key
> > lu_object_find
> > lu_site_hash_ops
> > lustre_attr_show
> > lustre_attr_store
> > lustre_fill_super
> > lustre_fs_type
> > lustre_init_lsi
> > lustre_mount
> > lustre_pack_reply_v2
> > lustre_profile_list
> > lustre_put_lsi
> > lustre_start_mgc
> > lustre_start_simple
> > lustre_swab_ldlm_lock_desc
> > lustre_swab_ldlm_policy_data
> > lustre_swab_ldlm_res_id
> > lustre_swab_ldlm_resource_desc
> > lustre_swab_ll_fid
> > lustre_swab_llog_id
> > lustre_swab_obdo
> > mgc_start_lock
> > nrs_policy_get_info_locked
> > obd_export_nid2str
> > obd_init_checks
> > obd_zombie_impexp_cull
> > ptl_get_pid
> > ptlrpc_interrupted_set
> > ptlrpc_new_bulk
> > ptlrpc_next_xid
> > ptlrpc_nrs_policy_register
> > ptlrpc_pinger_remove_timeouts
> > ptlrpc_register_bulk
> > ptlrpc_retain_replayable_request
> > ptlrpc_server_drop_request
> > ptlrpc_start_thread
> > ptlrpc_start_threads
> > ptlrpc_stop_all_threads
> > ptlrpc_unregister_reply
> > ptlrpcd_free
> > ptlrpcd_start
> > ptlrpcd_stop
> > req_capsule_field_present
> > req_capsule_init_area
> > server_name2fsname
> > server_name2index
> > sptlrpc_cli_wrap_bulk
> > sptlrpc_parse_rule
> > sptlrpc_req_replace_dead_ctx
> > sptlrpc_rule_set_choose
> > sptlrpc_rule_set_expand
> > sptlrpc_rule_set_free
> > sptlrpc_rule_set_merge
> > sptlrpc_sec_get
> > sptlrpc_secflags2str
> > sptlrpc_svc_install_rvs_ctx
> >
> >
>

Re: [Outreachy kernel] [PATCH 1/2] Staging: lustre: ptlrpc: Make functions static

[Shraddha Barke]

On Thu, 1 Oct 2015, Julia Lawall wrote:

> On Thu, 1 Oct 2015, Shraddha Barke wrote:
>
>>
>>
>> On Thu, 1 Oct 2015, Arnd Bergmann wrote:
>>
>>> On Thursday 01 October 2015 15:07:57 Shraddha Barke wrote:
>>>> Since these functions are not called anywhere apart from this particular
>>>> file, declare them as static.
>>>>
>>>> Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
>>>>
>>>
>>> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>>>
>>> Note that there are hundreds more symbols in lustre that should be made
>>> static.
>>> Most of them are harder to find because they have a declaration in a header
>>> file
>>> despite being only used in one place.
>
> Why does it make it hard to find?  One can make a spearate check in .c
> files.
>
> I would think though that the clean up could be a little bit tedious.  I
> guess that when the function is static, it shouldn't be declared in the
> header file any more?  At least not if the header file is shared
> between multipe files.  But it still may need to be declared somewhere, if
> it is referenced before it is declared in the one file where it is used.
>
> The question then is how to divide up the patches.  Should one just
> address all of the functions that have this property in a single file?  Or
> should one only work on a couple of functions at a time that seem related
> to each other?
>
> Another issue in Lustre is that there are a some EXPORT_SYMBOLs on
> functyions that are only used in one file.  Should they all be removed, or
> is it like the case of android where there is code not yet in the kernel
> that may rely on them?
>
> julia
>
>>> Coincidentally I've recently checked this for a subset of lustre while
>>> removing
>>> completely unused functions. Have a look at the list below to see if you can
>>> do more of those.
>>

Hello,
I've sent a sample patch. Would you please review it so that I can send 
more of them? And yes, how am I supposed to split the patches?

Thanks,

Shraddha

>> Yes I'll work on these.
>>
>> Thanks :)
>>>
>>> 	Arnd
>>>
>>> find build/tmp/drivers/staging/lustre/lustre/{ptlrpc,obdclass}/ -name \*.o |
>>> \
>>> 	grep -v built-in | xargs nm | grep -w [A-TV-Z] | grep -v __ksymtab |
>>> 	grep -v __crc | cut -f 3 -d \  | sort -u > defs
>>> find build/tmp/drivers/staging/lustre/ -name \*.o | grep -v built-in | \
>>> 	xargs nm | grep -w U | cut -f 11- -d\  | sort -u > refs
>>> diff -u defs refs | grep ^-  | cut -f 2 -d-
>>>
>>> __ptlrpc_free_bulk
>>> cache_stats_init
>>> cache_stats_print
>>> capa_add
>>> capa_cpy
>>> capa_decrypt_id
>>> capa_encrypt_id
>>> capa_hmac
>>> capa_lookup
>>> cl_env_peek
>>> cl_io_cancel
>>> cl_io_rw_advance
>>> cl_lock_enclosure
>>> cl_lock_extransit
>>> cl_lock_intransit
>>> cl_lock_mutex_try
>>> cl_lock_state_wait
>>> cl_page_list_assume
>>> cl_page_list_del
>>> cl_page_list_discard
>>> cl_page_list_fini
>>> cl_sync_io_init
>>> cl_sync_io_wait
>>> class_add_conn
>>> class_add_profile
>>> class_attach
>>> class_cleanup
>>> class_config_dump_handler
>>> class_config_parse_rec
>>> class_del_conn
>>> class_detach
>>> class_get_type
>>> class_match_param
>>> class_resolve_dev_name
>>> class_search_type
>>> class_setup
>>> cleanup_capa_hash
>>> deuuidify
>>> do_lcfg
>>> dt_acct_features
>>> dt_declare_version_set
>>> dt_device_fini
>>> dt_device_init
>>> dt_directory_features
>>> dt_find_or_create
>>> dt_global_fini
>>> dt_global_init
>>> dt_index_read
>>> dt_index_walk
>>> dt_key
>>> dt_lfsck_features
>>> dt_locate_at
>>> dt_lookup_dir
>>> dt_mode_to_dft
>>> dt_object_fini
>>> dt_object_init
>>> dt_otable_features
>>> dt_path_parser
>>> dt_quota_glb_features
>>> dt_quota_slv_features
>>> dt_read
>>> dt_record_read
>>> dt_record_write
>>> dt_store_open
>>> dt_store_resolve
>>> dt_try_as_dir
>>> dt_txn_callback_add
>>> dt_txn_callback_del
>>> dt_txn_hook_commit
>>> dt_txn_hook_start
>>> dt_txn_hook_stop
>>> dt_version_get
>>> dt_version_set
>>> dump_exports
>>> dump_obdo
>>> iattr_from_obdo
>>> init_capa_hash
>>> interval_expand
>>> interval_find
>>> interval_is_overlapped
>>> interval_iterate
>>> interval_iterate_reverse
>>> interval_search
>>> ldlm_add_bl_work_item
>>> ldlm_add_cp_work_item
>>> ldlm_blocking_ast
>>> ldlm_blocking_ast_nocheck
>>> ldlm_cancel_locks_for_export
>>> ldlm_cancel_locks_for_export_cb
>>> ldlm_cancel_lru_local
>>> ldlm_cli_cancel_req
>>> ldlm_cli_convert
>>> ldlm_cli_enqueue_local
>>> ldlm_cli_inactive_namespace_list
>>> ldlm_convert_policy_to_wire
>>> ldlm_destroy_export
>>> ldlm_dump_granted_max
>>> ldlm_enqueue_pack
>>> ldlm_errno2error
>>> ldlm_expired_completion_wait
>>> ldlm_flock_blocking_ast
>>> ldlm_get_enq_timeout
>>> ldlm_glimpse_ast
>>> ldlm_init_export
>>> ldlm_kobj
>>> ldlm_lock_add_to_lru
>>> ldlm_lock_add_to_lru_nolock
>>> ldlm_lock_destroy_internal
>>> ldlm_lock_downgrade
>>> ldlm_lock_fail_match
>>> ldlm_lock_touch_in_lru
>>> ldlm_namespace_foreach
>>> ldlm_namespace_free
>>> ldlm_namespace_get_return
>>> ldlm_namespace_register
>>> ldlm_namespace_sysfs_register
>>> ldlm_namespace_sysfs_unregister
>>> ldlm_namespace_unregister
>>> ldlm_ns_fid_hash_ops
>>> ldlm_ns_hash_defs
>>> ldlm_ns_hash_ops
>>> ldlm_pool_get_clv
>>> ldlm_pool_get_limit
>>> ldlm_pool_recalc
>>> ldlm_pool_set_limit
>>> ldlm_pool_set_slv
>>> ldlm_pool_setup
>>> ldlm_pool_shrink
>>> ldlm_pools_recalc
>>> ldlm_process_flock_lock
>>> ldlm_refresh_waiting_lock
>>> ldlm_register_intent
>>> ldlm_reprocess_all_ns
>>> ldlm_resource_foreach
>>> ldlm_resource_insert_lock_after
>>> ldlm_resource_putref_locked
>>> ldlm_svc_kset
>>> ldlm_typename
>>> ldlm_work_gl_ast_lock
>>> llog_cat_id2handle
>>> llog_cat_process_or_fork
>>> llog_thread_key
>>> lock_handle_ops
>>> lprocfs_dt_rd_blksize
>>> lprocfs_dt_rd_filesfree
>>> lprocfs_dt_rd_filestotal
>>> lprocfs_dt_rd_kbytesavail
>>> lprocfs_dt_rd_kbytesfree
>>> lprocfs_dt_rd_kbytestotal
>>> lprocfs_stats_seq_fops
>>> lprocfs_write_frac_u64_helper
>>> lu_context_key_quiesce
>>> lu_context_key_revive
>>> lu_context_refill
>>> lu_dev_add_linkage
>>> lu_device_put
>>> lu_global_key
>>> lu_object_find
>>> lu_site_hash_ops
>>> lustre_attr_show
>>> lustre_attr_store
>>> lustre_fill_super
>>> lustre_fs_type
>>> lustre_init_lsi
>>> lustre_mount
>>> lustre_pack_reply_v2
>>> lustre_profile_list
>>> lustre_put_lsi
>>> lustre_start_mgc
>>> lustre_start_simple
>>> lustre_swab_ldlm_lock_desc
>>> lustre_swab_ldlm_policy_data
>>> lustre_swab_ldlm_res_id
>>> lustre_swab_ldlm_resource_desc
>>> lustre_swab_ll_fid
>>> lustre_swab_llog_id
>>> lustre_swab_obdo
>>> mgc_start_lock
>>> nrs_policy_get_info_locked
>>> obd_export_nid2str
>>> obd_init_checks
>>> obd_zombie_impexp_cull
>>> ptl_get_pid
>>> ptlrpc_interrupted_set
>>> ptlrpc_new_bulk
>>> ptlrpc_next_xid
>>> ptlrpc_nrs_policy_register
>>> ptlrpc_pinger_remove_timeouts
>>> ptlrpc_register_bulk
>>> ptlrpc_retain_replayable_request
>>> ptlrpc_server_drop_request
>>> ptlrpc_start_thread
>>> ptlrpc_start_threads
>>> ptlrpc_stop_all_threads
>>> ptlrpc_unregister_reply
>>> ptlrpcd_free
>>> ptlrpcd_start
>>> ptlrpcd_stop
>>> req_capsule_field_present
>>> req_capsule_init_area
>>> server_name2fsname
>>> server_name2index
>>> sptlrpc_cli_wrap_bulk
>>> sptlrpc_parse_rule
>>> sptlrpc_req_replace_dead_ctx
>>> sptlrpc_rule_set_choose
>>> sptlrpc_rule_set_expand
>>> sptlrpc_rule_set_free
>>> sptlrpc_rule_set_merge
>>> sptlrpc_sec_get
>>> sptlrpc_secflags2str
>>> sptlrpc_svc_install_rvs_ctx
>>>
>>>
>>
>

Re: [Outreachy kernel] [PATCH 1/2] Staging: lustre: ptlrpc: Make functions static

[Arnd Bergmann]

On Thursday 01 October 2015 15:20:39 Julia Lawall wrote:
> On Thu, 1 Oct 2015, Shraddha Barke wrote:
> 
> >
> >
> > On Thu, 1 Oct 2015, Arnd Bergmann wrote:
> >
> > > On Thursday 01 October 2015 15:07:57 Shraddha Barke wrote:
> > > > Since these functions are not called anywhere apart from this particular
> > > > file, declare them as static.
> > > >
> > > > Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
> > > >
> > >
> > > Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> > >
> > > Note that there are hundreds more symbols in lustre that should be made
> > > static.
> > > Most of them are harder to find because they have a declaration in a header
> > > file
> > > despite being only used in one place.
> 
> Why does it make it hard to find?  One can make a spearate check in .c
> files.

We currently get a sparse warning for each global identifier definition
that is not preceded by a declaration, but since most of our tools
(gcc, sparse, coccinelle, ...) work on a single file at a time, they
do not know whether a global symbol that is defined in a C file and
(incorrectly) declared in a header is used in another C file or not.

In case of lustre, a lot of functions were originally global because they
were used in the server-side code. However, the code we have in staging
today only includes the client-side implementation and we want to
remove all traces of the server side from it for simplicity.

> I would think though that the clean up could be a little bit tedious.  I
> guess that when the function is static, it shouldn't be declared in the
> header file any more?

Correct. The first step could be to remove declarations for the symbols,
followed by actually removing the ones that we then get sparse warnings
for.

>  At least not if the header file is shared
> between multipe files.  But it still may need to be declared somewhere, if
> it is referenced before it is declared in the one file where it is used.

I would also consider that a bug we should warn about: basically any
'extern' variable declaration and any forward declaration for a 
global function (with or without the redundant extern specifier) should
be considered a bug. We want the same declaration to be visible by both
the file that defines it and the file that uses it so we get warned about
type mismatches.

> The question then is how to divide up the patches.  Should one just
> address all of the functions that have this property in a single file?  Or
> should one only work on a couple of functions at a time that seem related
> to each other?

I think either way would be fine.

There is one more thing to look out for: if a symbol is only used in
the file in which it gets defined, but the use comes before the
definition, we still need a 'static' forward declaration. Ideally
the functions should be reordered in a way that avoids those declarations,
but we don't want patches that mix a large scale move of code around
a file with a simpler change to make the symbols static.

The two parts can be done in either order: we could mark the functions
static and add the forward declarations in one patch, and move them
to their proper place in a follow-up, or do the reverse without the
forward declaration.

> Another issue in Lustre is that there are a some EXPORT_SYMBOLs on
> functyions that are only used in one file.  Should they all be removed, or
> is it like the case of android where there is code not yet in the kernel
> that may rely on them?

They should all be removed as far as I can tell. It's a bug to export
a static symbol. I don't think that happens a lot, but this is something
we could easily check for using coccinelle.

	Arnd

Re: [Outreachy kernel] [PATCH 1/2] Staging: lustre: ptlrpc: Make functions static

[Julia Lawall]

On Thu, 1 Oct 2015, Arnd Bergmann wrote:

> On Thursday 01 October 2015 15:20:39 Julia Lawall wrote:
> > On Thu, 1 Oct 2015, Shraddha Barke wrote:
> >
> > >
> > >
> > > On Thu, 1 Oct 2015, Arnd Bergmann wrote:
> > >
> > > > On Thursday 01 October 2015 15:07:57 Shraddha Barke wrote:
> > > > > Since these functions are not called anywhere apart from this particular
> > > > > file, declare them as static.
> > > > >
> > > > > Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
> > > > >
> > > >
> > > > Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> > > >
> > > > Note that there are hundreds more symbols in lustre that should be made
> > > > static.
> > > > Most of them are harder to find because they have a declaration in a header
> > > > file
> > > > despite being only used in one place.
> >
> > Why does it make it hard to find?  One can make a spearate check in .c
> > files.
>
> We currently get a sparse warning for each global identifier definition
> that is not preceded by a declaration, but since most of our tools
> (gcc, sparse, coccinelle, ...) work on a single file at a time, they
> do not know whether a global symbol that is defined in a C file and
> (incorrectly) declared in a header is used in another C file or not.

OK, but from Coccinlle you can just call grep, and check the extensions on
the returned file names, so it is not a big problem.

> In case of lustre, a lot of functions were originally global because they
> were used in the server-side code. However, the code we have in staging
> today only includes the client-side implementation and we want to
> remove all traces of the server side from it for simplicity.

OK.

> > I would think though that the clean up could be a little bit tedious.  I
> > guess that when the function is static, it shouldn't be declared in the
> > header file any more?
>
> Correct. The first step could be to remove declarations for the symbols,
> followed by actually removing the ones that we then get sparse warnings
> for.
>
> >  At least not if the header file is shared
> > between multipe files.  But it still may need to be declared somewhere, if
> > it is referenced before it is declared in the one file where it is used.
>
> I would also consider that a bug we should warn about: basically any
> 'extern' variable declaration and any forward declaration for a
> global function (with or without the redundant extern specifier) should
> be considered a bug. We want the same declaration to be visible by both
> the file that defines it and the file that uses it so we get warned about
> type mismatches.

I think there would be warning about type mismatches anyway, if the
prototype doesn't match with the definition?

> > The question then is how to divide up the patches.  Should one just
> > address all of the functions that have this property in a single file?  Or
> > should one only work on a couple of functions at a time that seem related
> > to each other?
>
> I think either way would be fine.

OK.  Then from my experience, I think that doing a few related functions
at once would be easier.

> There is one more thing to look out for: if a symbol is only used in
> the file in which it gets defined, but the use comes before the
> definition, we still need a 'static' forward declaration. Ideally
> the functions should be reordered in a way that avoids those declarations,
> but we don't want patches that mix a large scale move of code around
> a file with a simpler change to make the symbols static.
>
> The two parts can be done in either order: we could mark the functions
> static and add the forward declarations in one patch, and move them
> to their proper place in a follow-up, or do the reverse without the
> forward declaration.
>
> > Another issue in Lustre is that there are a some EXPORT_SYMBOLs on
> > functyions that are only used in one file.  Should they all be removed, or
> > is it like the case of android where there is code not yet in the kernel
> > that may rely on them?
>
> They should all be removed as far as I can tell. It's a bug to export
> a static symbol. I don't think that happens a lot, but this is something
> we could easily check for using coccinelle.

I cleaned up a bunch of them at one point.  I don't seem to have
integrated the semantic patch into the kernel though...

julia

Re: [Outreachy kernel] [PATCH 1/2] Staging: lustre: ptlrpc: Make functions static

[Arnd Bergmann]

On Thursday 01 October 2015 15:49:21 Julia Lawall wrote:
> On Thu, 1 Oct 2015, Arnd Bergmann wrote:
> > On Thursday 01 October 2015 15:20:39 Julia Lawall wrote:
> > > On Thu, 1 Oct 2015, Shraddha Barke wrote:
> > > > > Note that there are hundreds more symbols in lustre that should be made
> > > > > static.
> > > > > Most of them are harder to find because they have a declaration in a header
> > > > > file
> > > > > despite being only used in one place.
> > >
> > > Why does it make it hard to find?  One can make a spearate check in .c
> > > files.
> >
> > We currently get a sparse warning for each global identifier definition
> > that is not preceded by a declaration, but since most of our tools
> > (gcc, sparse, coccinelle, ...) work on a single file at a time, they
> > do not know whether a global symbol that is defined in a C file and
> > (incorrectly) declared in a header is used in another C file or not.
> 
> OK, but from Coccinlle you can just call grep, and check the extensions on
> the returned file names, so it is not a big problem.

I'm not following here. How exactly would you find symbols like this with coccinelle?

subsystem.h:

extern int subsystem_state;

subsystem.c:

#include "subsystem.h"
long subsystem_state;

static void function(void)
{
	...
	subsystem_state++;
	...
}

with no users of this variable outside of this file.

> > >  At least not if the header file is shared
> > > between multipe files.  But it still may need to be declared somewhere, if
> > > it is referenced before it is declared in the one file where it is used.
> >
> > I would also consider that a bug we should warn about: basically any
> > 'extern' variable declaration and any forward declaration for a
> > global function (with or without the redundant extern specifier) should
> > be considered a bug. We want the same declaration to be visible by both
> > the file that defines it and the file that uses it so we get warned about
> > type mismatches.
> 
> I think there would be warning about type mismatches anyway, if the
> prototype doesn't match with the definition?

The point is that this only works if all files include the same header.

e.g.

subsystem.h:

extern int subsystem_state;

subsystem.c:

#include "subsystem.h"
long subsystem_state;

driver.c:
/* no #include subsystem.h */
extern long subsystem_state;

	Arnd

Re: [Outreachy kernel] [PATCH 1/2] Staging: lustre: ptlrpc: Make functions static

[Julia Lawall]

On Thu, 1 Oct 2015, Arnd Bergmann wrote:

> On Thursday 01 October 2015 15:49:21 Julia Lawall wrote:
> > On Thu, 1 Oct 2015, Arnd Bergmann wrote:
> > > On Thursday 01 October 2015 15:20:39 Julia Lawall wrote:
> > > > On Thu, 1 Oct 2015, Shraddha Barke wrote:
> > > > > > Note that there are hundreds more symbols in lustre that should be made
> > > > > > static.
> > > > > > Most of them are harder to find because they have a declaration in a header
> > > > > > file
> > > > > > despite being only used in one place.
> > > >
> > > > Why does it make it hard to find?  One can make a spearate check in .c
> > > > files.
> > >
> > > We currently get a sparse warning for each global identifier definition
> > > that is not preceded by a declaration, but since most of our tools
> > > (gcc, sparse, coccinelle, ...) work on a single file at a time, they
> > > do not know whether a global symbol that is defined in a C file and
> > > (incorrectly) declared in a header is used in another C file or not.
> >
> > OK, but from Coccinlle you can just call grep, and check the extensions on
> > the returned file names, so it is not a big problem.
>
> I'm not following here. How exactly would you find symbols like this with coccinelle?
>
> subsystem.h:
>
> extern int subsystem_state;
>
> subsystem.c:
>
> #include "subsystem.h"
> long subsystem_state;
>
> static void function(void)
> {
> 	...
> 	subsystem_state++;
> 	...
> }
>
> with no users of this variable outside of this file.

The following uses ocaml, because I'm not very good at python...

@r disable optional_storage@
identifier f;
position p;
@@

f@p(...) { ... }

@script:ocaml@
p << r.p;
f << r.f;
@@

let l = Common,cmd_to_list
          (Printf.sprintf "cd /my/linux/path; git grep %s" f) in
let other_files =
  List.map (function x -> List.hd(Str.split (Str.regexp ":") x)) l in
let file = (List.hd p).file in
match List.filter (function x -> not(x = file)) other_files with
  [] -> Printf.printf "function %s is only used in file %s\n" f file
| _ -> ()

That is, find the definition of a function with the pattern matching rule,
then obtain the name and position of the function definition, then in the
ocaml/python rule, grep for the name of the function, and extract the
names of the files from grep.  Drop all the names that are the same as
that of the file containing the definition, and print an error message if
the result is an empty list.

The disable optional_storage at the beginning of the first rule prevents
the function definition from matching a static function, since static
isn't mentioned in the pattern.

This takes care of the case of function definitions.  Something similar
could probably be done for other kinds fo variables.  That could be a bit
more awkward, though because variable declarations both inside and outside
function definitions look alike.

> > > >  At least not if the header file is shared
> > > > between multipe files.  But it still may need to be declared somewhere, if
> > > > it is referenced before it is declared in the one file where it is used.
> > >
> > > I would also consider that a bug we should warn about: basically any
> > > 'extern' variable declaration and any forward declaration for a
> > > global function (with or without the redundant extern specifier) should
> > > be considered a bug. We want the same declaration to be visible by both
> > > the file that defines it and the file that uses it so we get warned about
> > > type mismatches.
> >
> > I think there would be warning about type mismatches anyway, if the
> > prototype doesn't match with the definition?
>
> The point is that this only works if all files include the same header.
>
> e.g.
>
> subsystem.h:
>
> extern int subsystem_state;
>
> subsystem.c:
>
> #include "subsystem.h"
> long subsystem_state;
>
> driver.c:
> /* no #include subsystem.h */
> extern long subsystem_state;

OK, indeed getting the definition from different places is quite error
prone.

julia

> 	Arnd
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/4003012.PLAPRmBvBo%40wuerfel.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH 1/2] Staging: lustre: ptlrpc: Make functions static

[Arnd Bergmann]

On Thursday 01 October 2015 16:12:53 Julia Lawall wrote:
> On Thu, 1 Oct 2015, Arnd Bergmann wrote:
> > On Thursday 01 October 2015 15:49:21 Julia Lawall wrote:
> > > On Thu, 1 Oct 2015, Arnd Bergmann wrote:
> > > > On Thursday 01 October 2015 15:20:39 Julia Lawall wrote:
> > > > > On Thu, 1 Oct 2015, Shraddha Barke wrote:
> > > > > > > Note that there are hundreds more symbols in lustre that should be made
> > > > > > > static.
> > > > > > > Most of them are harder to find because they have a declaration in a header
> > > > > > > file
> > > > > > > despite being only used in one place.
> > > > >
> > > > > Why does it make it hard to find?  One can make a spearate check in .c
> > > > > files.
> > > >
> > > > We currently get a sparse warning for each global identifier definition
> > > > that is not preceded by a declaration, but since most of our tools
> > > > (gcc, sparse, coccinelle, ...) work on a single file at a time, they
> > > > do not know whether a global symbol that is defined in a C file and
> > > > (incorrectly) declared in a header is used in another C file or not.
> > >
> > > OK, but from Coccinlle you can just call grep, and check the extensions on
> > > the returned file names, so it is not a big problem.
> >
> > I'm not following here. How exactly would you find symbols like this with coccinelle?
> >
> > subsystem.h:
> >
> > extern int subsystem_state;
> >
> > subsystem.c:
> >
> > #include "subsystem.h"
> > long subsystem_state;
> >
> > static void function(void)
> > {
> > 	...
> > 	subsystem_state++;
> > 	...
> > }
> >
> > with no users of this variable outside of this file.
> 
> The following uses ocaml, because I'm not very good at python...
> 
> @r disable optional_storage@
> identifier f;
> position p;
> @@
> 
> f@p(...) { ... }
> 
> @script:ocaml@
> p << r.p;
> f << r.f;
> @@
> 
> let l = Common,cmd_to_list
>           (Printf.sprintf "cd /my/linux/path; git grep %s" f) in
> let other_files =
>   List.map (function x -> List.hd(Str.split (Str.regexp ":") x)) l in
> let file = (List.hd p).file in
> match List.filter (function x -> not(x = file)) other_files with
>   [] -> Printf.printf "function %s is only used in file %s\n" f file
> | _ -> ()
> 
> That is, find the definition of a function with the pattern matching rule,
> then obtain the name and position of the function definition, then in the
> ocaml/python rule, grep for the name of the function, and extract the
> names of the files from grep.  Drop all the names that are the same as
> that of the file containing the definition, and print an error message if
> the result is an empty list.
> 
> The disable optional_storage at the beginning of the first rule prevents
> the function definition from matching a static function, since static
> isn't mentioned in the pattern.
> 
> This takes care of the case of function definitions.  Something similar
> could probably be done for other kinds fo variables.  That could be a bit
> more awkward, though because variable declarations both inside and outside
> function definitions look alike.

Ok, I see. Yes, this looks nice, though there might still be a small number
of false positives (e.g. identifier used in inline function in the header
that declares it, inline function used elsewhere) and false negatives
(function name mentioned in a .c file comment elsewhere but not actually
called).

I don't actually understand half the code you write, but conceptually
it makes sense and is similar to what I did manually. If you plan to
actually make use of this method, there are a couple of improvements
that I can think of:

- "git grep -w" to match only whole words
- "git grep -l" to list the file names only, rather than manually parsing
  the default grep output
- filter out declarations from header files as you mentioned (I don't
  see this in your code).

	Arnd

Re: [Outreachy kernel] [PATCH 1/2] Staging: lustre: ptlrpc: Make functions static

[Julia Lawall]

On Thu, 1 Oct 2015, Arnd Bergmann wrote:

> On Thursday 01 October 2015 16:12:53 Julia Lawall wrote:
> > On Thu, 1 Oct 2015, Arnd Bergmann wrote:
> > > On Thursday 01 October 2015 15:49:21 Julia Lawall wrote:
> > > > On Thu, 1 Oct 2015, Arnd Bergmann wrote:
> > > > > On Thursday 01 October 2015 15:20:39 Julia Lawall wrote:
> > > > > > On Thu, 1 Oct 2015, Shraddha Barke wrote:
> > > > > > > > Note that there are hundreds more symbols in lustre that should be made
> > > > > > > > static.
> > > > > > > > Most of them are harder to find because they have a declaration in a header
> > > > > > > > file
> > > > > > > > despite being only used in one place.
> > > > > >
> > > > > > Why does it make it hard to find?  One can make a spearate check in .c
> > > > > > files.
> > > > >
> > > > > We currently get a sparse warning for each global identifier definition
> > > > > that is not preceded by a declaration, but since most of our tools
> > > > > (gcc, sparse, coccinelle, ...) work on a single file at a time, they
> > > > > do not know whether a global symbol that is defined in a C file and
> > > > > (incorrectly) declared in a header is used in another C file or not.
> > > >
> > > > OK, but from Coccinlle you can just call grep, and check the extensions on
> > > > the returned file names, so it is not a big problem.
> > >
> > > I'm not following here. How exactly would you find symbols like this with coccinelle?
> > >
> > > subsystem.h:
> > >
> > > extern int subsystem_state;
> > >
> > > subsystem.c:
> > >
> > > #include "subsystem.h"
> > > long subsystem_state;
> > >
> > > static void function(void)
> > > {
> > > 	...
> > > 	subsystem_state++;
> > > 	...
> > > }
> > >
> > > with no users of this variable outside of this file.
> > 
> > The following uses ocaml, because I'm not very good at python...
> > 
> > @r disable optional_storage@
> > identifier f;
> > position p;
> > @@
> > 
> > f@p(...) { ... }
> > 
> > @script:ocaml@
> > p << r.p;
> > f << r.f;
> > @@
> > 
> > let l = Common,cmd_to_list
> >           (Printf.sprintf "cd /my/linux/path; git grep %s" f) in
> > let other_files =
> >   List.map (function x -> List.hd(Str.split (Str.regexp ":") x)) l in
> > let file = (List.hd p).file in
> > match List.filter (function x -> not(x = file)) other_files with
> >   [] -> Printf.printf "function %s is only used in file %s\n" f file
> > | _ -> ()
> > 
> > That is, find the definition of a function with the pattern matching rule,
> > then obtain the name and position of the function definition, then in the
> > ocaml/python rule, grep for the name of the function, and extract the
> > names of the files from grep.  Drop all the names that are the same as
> > that of the file containing the definition, and print an error message if
> > the result is an empty list.
> > 
> > The disable optional_storage at the beginning of the first rule prevents
> > the function definition from matching a static function, since static
> > isn't mentioned in the pattern.
> > 
> > This takes care of the case of function definitions.  Something similar
> > could probably be done for other kinds fo variables.  That could be a bit
> > more awkward, though because variable declarations both inside and outside
> > function definitions look alike.
> 
> Ok, I see. Yes, this looks nice, though there might still be a small number
> of false positives (e.g. identifier used in inline function in the header
> that declares it, inline function used elsewhere) and false negatives
> (function name mentioned in a .c file comment elsewhere but not actually
> called).

Yes.

> I don't actually understand half the code you write, but conceptually
> it makes sense and is similar to what I did manually. If you plan to
> actually make use of this method, there are a couple of improvements
> that I can think of:
> 
> - "git grep -w" to match only whole words
> - "git grep -l" to list the file names only, rather than manually parsing
>   the default grep output

Yes, thanks.

> - filter out declarations from header files as you mentioned (I don't
>   see this in your code).

Indeed.  The definitions are all in .c files, because that is what 
Coccinelle looks at by default, but indeed, the final match should only be 
on .c files.

thanks,
julia

> 	Arnd
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/18050482.hzx1IbEyMT%40wuerfel.
> For more options, visit https://groups.google.com/d/optout.
> 

Re: [Outreachy kernel] [PATCH 1/2] Staging: lustre: ptlrpc: Make functions static

[Sudip Mukherjee]

On Thu, Oct 01, 2015 at 03:07:57PM +0530, Shraddha Barke wrote:
> Since these functions are not called anywhere apart from this particular
> file, declare them as static.
> 
> Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
> ---
This has already been done by:
2adf5e877537 ("staging: lustre: make non-exported functions static")

regards
sudip

[PATCH 2/2] Staging: lustre: obdclass: Declare structure as static.

[Shraddha Barke]

Declare structure as static since it is not accessed anywhere
apart from this particular file.

Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
---
 drivers/staging/lustre/lustre/obdclass/lu_object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/lu_object.c b/drivers/staging/lustre/lustre/obdclass/lu_object.c
index c892e82..897c089 100644
--- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
+++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c
@@ -440,7 +440,7 @@ LU_KEY_INIT_FINI(lu_global, struct lu_cdebug_data);
  * Key, holding temporary buffer. This key is registered very early by
  * lu_global_init().
  */
-struct lu_context_key lu_global_key = {
+static struct lu_context_key lu_global_key = {
 	.lct_tags = LCT_MD_THREAD | LCT_DT_THREAD |
 		    LCT_MG_THREAD | LCT_CL_THREAD | LCT_LOCAL,
 	.lct_init = lu_global_key_init,
-- 
2.1.4

Re: [Outreachy kernel] [PATCH 2/2] Staging: lustre: obdclass: Declare structure as static.

[Arnd Bergmann]

On Thursday 01 October 2015 15:07:58 Shraddha Barke wrote:
> Declare structure as static since it is not accessed anywhere
> apart from this particular file.
> 
> Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
> 

Reviewed-by: Arnd Bergmann <arnd@arndb.de>