On 08/24/2017 02:58 PM, Oliver Hartkopp wrote:
> Naming structs and its instances identically is bad.
> 
> On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
> 
>>   struct netns_can {
>>   #if IS_ENABLED(CONFIG_PROC_FS)
>> @@ -30,8 +30,8 @@ struct netns_can {
>>   	struct dev_rcv_lists *can_rx_alldev_list;
>>   	spinlock_t can_rcvlists_lock;
>>   	struct timer_list can_stattimer;/* timer for statistics update */
>> -	struct s_stats *can_stats;	/* packet statistics */
>> -	struct s_pstats *can_pstats;	/* receive list statistics */
>> +	struct can_stats *can_stats; /* packet statistics */
>> +	struct can_pstats *can_pstats; /* receive list statistics */
                               ^
Now I found out what that "p" originally means:

> /* persistent statistics */
> struct can_pstats {
> 	unsigned long stats_reset;
> 	unsigned long user_reset;
> 	unsigned long rcv_entries;
> 	unsigned long rcv_entries_max;
> };

\o/

> If you really want to rename this stuff, what about this:
> 
> struct can_pkt_stats *can_stats; /* packet statistics */
> struct can_rxl_stats *can_rstats; /* receive list statistics */
             ^^^
We're using rcv_list(s) instead of rxl in various other places.

If we give the variables speaking names, the comments are obsolete:

struct can_pkg_stats *can_pkg_stats;
struct can_rcv_list_stats *can_rcv_list_stats;

We can remove the "can_" prefix of the variables in the struct
netns_can, as the struct already has the "can" in it.

So the struct becomes:

> struct netns_can {
> #if IS_ENABLED(CONFIG_PROC_FS)
> 	struct proc_dir_entry *proc_dir;
> 	struct proc_dir_entry *pde_version;
> 	struct proc_dir_entry *pde_stats;
> 	struct proc_dir_entry *pde_reset_stats;
> 	struct proc_dir_entry *pde_rcvlist_all;
> 	struct proc_dir_entry *pde_rcvlist_fil;
> 	struct proc_dir_entry *pde_rcvlist_inv;
> 	struct proc_dir_entry *pde_rcvlist_sff;
> 	struct proc_dir_entry *pde_rcvlist_eff;
> 	struct proc_dir_entry *pde_rcvlist_err;
> 	struct proc_dir_entry *bcmproc_dir;
> #endif
> 
> 	/* receive filters subscribed for 'all' CAN devices */
> 	struct can_dev_rcv_lists *rx_alldev_list;
> 	spinlock_t rcvlists_lock;
> 	struct timer_list stattimer; /* timer for statistics update */
> 	struct can_pkg_stats *pkg_stats;
> 	struct can_rcv_lists_stats *rcv_lists_stats;
> 
> #if IS_ENABLED(CONFIG_CAN_GW)
> 	/* CAN GW per-net gateway jobs */
> 	struct hlist_head cgw_list;
> #endif
> };

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |