netdev - Re: [PATCH] Staging: gdm724x: gdm

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1394998281.5689.63.camel@joe-AO722>
Date:	Sun, 16 Mar 2014 12:31:21 -0700
From:	Joe Perches <joe@...ches.com>
To:	Daniel Ngu <daniel.dy.ngu@...il.com>
Cc:	Jonathan Kim <jonathankim@...semi.com>,
	Dean ahn <deanahn@...semi.com>, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] Staging: gdm724x: gdm_lte.c: fixed coding style

On Sun, 2014-03-16 at 12:24 -0700, Joe Perches wrote:

> > @@ -551,28 +583,41 @@ static void gdm_lte_netif_rx(struct net_device *dev, char *buf, int len, int fla
> []
> >  			struct dhcp_packet {
> 
> This one would be better fixed by moving the struct definition
> out of this function altogether.
> 
> >  				u8 op;      /* BOOTREQUEST or BOOTREPLY */
> > -				u8 htype;   /* hardware address type. 1 = 10mb ethernet */
> > +				u8 htype;   /* hardware address type.
> > +					       1 = 10mb ethernet */
> >  				u8 hlen;    /* hardware address length */
> >  				u8 hops;    /* used by relay agents only */
> >  				u32 xid;    /* unique id */
> > -				u16 secs;   /* elapsed since client began acquisition/renewal */
> > +				u16 secs;   /* elapsed since client began
> > +					       acquisition/renewal */
> >  				u16 flags;  /* only one flag so far: */
> > -				#define BROADCAST_FLAG 0x8000 /* "I need broadcast replies" */
> > -				u32 ciaddr; /* client IP (if client is in BOUND, RENEW or REBINDING state) */
> > +				#define BROADCAST_FLAG 0x8000 /* "I need
> > +								 broadcast
> > +								 replies" */
> > +				u32 ciaddr; /* client IP (if client is in
> > +					       BOUND, RENEW or REBINDING
> > +					       state) */
> >  				u32 yiaddr; /* 'your' (client) IP address */
> > -				/* IP address of next server to use in bootstrap, returned in DHCPOFFER, DHCPACK by server */
> > +				/* IP address of next server to use in
> > +				 * bootstrap, returned in DHCPOFFER, DHCPACK by
> > +				 * server */
> >  				u32 siaddr_nip;
> >  				u32 gateway_nip; /* relay agent IP address */
> > -				u8 chaddr[16];   /* link-layer client hardware address (MAC) */
> > +				u8 chaddr[16];   /* link-layer client hardware
> > +						    address (MAC) */
> >  				u8 sname[64];    /* server host name (ASCIZ) */
> >  				u8 file[128];    /* boot file name (ASCIZ) */
> > -				u32 cookie;      /* fixed first four option bytes (99,130,83,99 dec) */
> > +				u32 cookie;      /* fixed first four option
> > +						    bytes (99,130,83,99 dec) */
> >  			} __packed;

Also, the net/ipv4/ipconfig.c definition
seems a lot more sensible as it uses endian
specific types

struct bootp_pkt {		/* BOOTP packet format */
	struct iphdr iph;	/* IP header */
	struct udphdr udph;	/* UDP header */
	u8 op;			/* 1=request, 2=reply */
	u8 htype;		/* HW address type */
	u8 hlen;		/* HW address length */
	u8 hops;		/* Used only by gateways */
	__be32 xid;		/* Transaction ID */
	__be16 secs;		/* Seconds since we started */
	__be16 flags;		/* Just what it says */
	__be32 client_ip;		/* Client's IP address if known */
	__be32 your_ip;		/* Assigned IP address */
	__be32 server_ip;		/* (Next, e.g. NFS) Server's IP address */
	__be32 relay_ip;		/* IP address of BOOTP relay */
	u8 hw_addr[16];		/* Client's HW address */
	u8 serv_name[64];	/* Server host name */
	u8 boot_file[128];	/* Name of boot file */
	u8 exten[312];		/* DHCP options / BOOTP vendor extensions */
};

It'd probably be better to make that struct definition public
and use it instead.

--
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