netdev - Re: [PATCH 2/7] CAN: Add PF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	15 Nov 2007 12:36:02 +0100
From:	Urs Thuermann <urs.thuermann@....de>
To:	Stephen Hemminger <shemminger@...ux-foundation.org>
Cc:	netdev@...r.kernel.org, David Miller <davem@...emloft.net>,
	Patrick McHardy <kaber@...sh.net>,
	Oliver Hartkopp <oliver.hartkopp@...kswagen.de>
Subject: Re: [PATCH 2/7] CAN: Add PF_CAN core module

Stephen Hemminger <shemminger@...ux-foundation.org> writes:

> > +#ifdef CONFIG_CAN_DEBUG_CORE
> > +extern void can_debug_skb(struct sk_buff *skb);
> > +extern void can_debug_cframe(const char *msg, struct can_frame *cframe);
> > +#define DBG(fmt, args...)  (DBG_VAR & 1 ? printk( \
> > +					KERN_DEBUG DBG_PREFIX ": %s: " fmt, \
> > +					__func__, ##args) : 0)
> > +#define DBG_FRAME(fmt, cf) (DBG_VAR & 2 ? can_debug_cframe(fmt, cf) : 0)
> > +#define DBG_SKB(skb)       (DBG_VAR & 4 ? can_debug_skb(skb) : 0)
> > +#else
> > +#define DBG(fmt, args...)
> > +#define DBG_FRAME(fmt, cf)
> > +#define DBG_SKB(skb)
> > +#endif
> 
> 
> This non-standard debugging seems like it needs a better interface.
> Also, need paren's around (DBG_VAR & 1) and don't use UPPERCASE for
> variable names.

No additional parenthesis is needed here.  ?: is the lowest precedence
operator above assignment and ,.  Also, DBG_VAR is no variable name.
It's a macro that expands to a variable name like can_debug, raw_debug
or bcm_debug.

> > +HLIST_HEAD(rx_dev_list);
> 
> Please either make rx_dev_list static or call it can_rx_dev_list
> to avoid name conflices.
> 
> 
> > +static struct dev_rcv_lists rx_alldev_list;
> > +static DEFINE_SPINLOCK(rcv_lists_lock);
> > +
> > +static struct kmem_cache *rcv_cache __read_mostly;
> > +
> > +/* table of registered CAN protocols */
> > +static struct can_proto *proto_tab[CAN_NPROTO] __read_mostly;
> > +static DEFINE_SPINLOCK(proto_tab_lock);
> > +
> > +struct timer_list stattimer; /* timer for statistics update */
> > +struct s_stats  stats;       /* packet statistics */
> > +struct s_pstats pstats;      /* receive list statistics */
> 
> More global variables without prefix.

These variables are not exported with EXPORT_SYMBOL, so there should
be no name conflict.  They cannot be made static because they are used
in af_can.c and proc.c.  Nevertheless we can prefix them with can_ if
you still think it's necessary.

> > +static int can_proc_read_stats(char *page, char **start, off_t off,
> > +			       int count, int *eof, void *data)
> > +{

> > +}
> 
> The read interface should use seq_file interface rather than
> formatting into page buffer.

Why?  For this simple function a page buffer is enough space and the
seq_file API would require more effort.  IMHO, seq_files offer
advantages if the proc file shows some sequence of data generated in
an iteration through some loop (see below).

> > +static int can_proc_read_reset_stats(char *page, char **start, off_t off,
> > +				     int count, int *eof, void *data)
> > +{

> > +}
> 
> Why not have a write interface to do the reset?

I haven't looked into writable proc files yet.  Will do so.

> > +static int can_proc_read_rcvlist(char *page, char **start, off_t off,
> > +				 int count, int *eof, void *data)
> > +{
> > +	/* double cast to prevent GCC warning */
> > +	int idx = (int)(long)data;

> > +}

This is were I would prefer sequence files.  However, the seq file
interface doesn't allow me to pass additional info like the `data'
argument.  This means I would have to write separate functions
instead.

> Output from checkpatch:
> 
> WARNING: do not add new typedefs
> #116: FILE: include/linux/can.h:41:
> +typedef __u32 canid_t;
> 
> WARNING: do not add new typedefs
> #124: FILE: include/linux/can.h:49:
> +typedef __u32 can_err_mask_t;

These typedef were considered OK in previous discussions on the list.

> ERROR: use tabs not spaces
> #498: FILE: net/can/af_can.c:159:
> +^I^I^I^I        " not implemented.\n", module_name);$

Fixed.

> WARNING: braces {} are not necessary for single statement blocks
> #1080: FILE: net/can/af_can.c:741:
> +	if (!proto_tab[proto]) {
> +		printk(KERN_ERR "BUG: can: protocol %d is not registered\n",
> +		       proto);
> +	}

Hm, isn't it common to use braces for single statements if they span
more than one line?

Thanks for your review.

urs
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html