linux-kernel - Re: [PATCH] ipwireless: driver for 3G PC Card

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200801291440.06099.dsterba@suse.cz>
Date:	Tue, 29 Jan 2008 14:40:05 +0100
From:	David Sterba <dsterba@...e.cz>
To:	Randy Dunlap <rdunlap@...otime.net>
Cc:	torvalds@...ux-foundation.org, linux-kernel@...r.kernel.org,
	jkosina@...e.cz, benm@...metric.co.nz, stephen@...metric.co.nz
Subject: Re: [PATCH] ipwireless: driver for 3G PC Card

On Monday 28 of January 2008 19:08:54 Randy Dunlap wrote:

> > +/* Number of bytes in NL packet header (can not do
>
> cannot

Fixed.

>
> > + * sizeof(nl_packet_header) since it's a bitfield) */
> > +#define NL_FOLLOWING_PACKET_HEADER_SIZE    1
> > +
> > +struct nl_first_paket_header {
>
>                    packet ?

Yes, packet of course.

>
> > +#if defined(__BIG_ENDIAN)
> > +	unsigned char packet_rank:2;
> > +	unsigned char address:3;
> > +	unsigned char protocol:3;
> > +#else
> > +	unsigned char protocol:3;
> > +	unsigned char address:3;
> > +	unsigned char packet_rank:2;
> > +#endif
>
> From C99 spec:
> "The order of allocation of bit-fields within a unit (high-order to
> low-order or low-order to high-order) is implementation-defined."
>
> so if the order/location of these bitfields is important (from one
> system to another), you should use bit masks instead of bitfields
> for them.

I've changed it to __BIG_ENDIAN_BITFIELDS, as suggested by Alexey. The order 
is important. I'll add it to my todo to convert it to bitmasks.

> > +/*
> > + * @return 1 if something has been received from hw
>
> What's with the '@'?

Removed, and converted to plaintext as suggested by others.

> Need ":" and/or space between CARD_NAME and following string.
> (in several places)

Found a few of them and fixed.

> > +module_param(major, int, 0);
> > +module_param(ipwireless_debug, int, 0);
> > +module_param(ipwireless_loopback, int, 0);
> > +module_param(ipwireless_out_queue, int, 0);
> > +MODULE_PARM_DESC(major, "ttyIPWp major number [0]");
> > +MODULE_PARM_DESC(ipwireless_debug, "switch on debug messages [0]");
> > +MODULE_PARM_DESC(ipwireless_debug, "switch on loopback mode [0]");
> > +MODULE_PARM_DESC(ipwireless_debug, "set size of outgoing queue [1]");
>
> Will these parameters be documented anywhere?

The options are intended for debugging. Normal user does not need to tweak 
them. Adjusted the description to reflect their debugging purpose.

> > +#ifdef IPWIRELESS_STATE_DEBUG
> > +int ipwireless_dump_network_state(char *p, struct ipw_network *network)
> > +{
> > +	int idx = 0;
> > +
> > +	idx += sprintf(p + idx, "debug: ppp_blocked=%d\n",
> > +			network->ppp_blocked);
> > +	idx += sprintf(p + idx, "debug: outgoing_packets_queued=%d\n",
> > +			network->outgoing_packets_queued);
> > +	idx += sprintf(p + idx, "debug: network.shutting_down=%d\n",
> > +			network->shutting_down);
>
> check for overflow of 'p'?

Converted to one snprintf too.

> > +/* Syncronous start-messages */
>
> 	Synchronous

Fixed.

Updated patch will follow.


Thank you for comments.

Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/