Re: [PATCH v4 2/6] Sim Access Profile Server

[Johan Hedberg <johan.hedberg@gmail.com>]

Hi Waldek,

On Wed, Mar 16, 2011, Waldemar.Rymarkiewicz@tieto.com wrote:
> >I've pushed the first two patches in this set, but it's gonna 
> >take quite a bit longer to properly review the rest (at a 
> >quick glance I noticed at least some coding style stuff with 
> >over 80-character lines).
> 
> Yes, I guess you refer  to eg.
> 
> case SAP_SET_TRANSPORT_PROTOCOL_REQ:
> 	if (msg->nparam == 0x01 &&
> 			msg->param->id == SAP_PARAM_ID_TRANSPORT_PROTOCOL &&
> 			ntohs(msg->param->len) == SAP_PARAM_ID_TRANSPORT_PROTOCOL_LEN &&
> 			(*msg->param->val  == SAP_TRANSPORT_PROTOCOL_T0 ||
> 			*msg->param->val == SAP_TRANSPORT_PROTOCOL_T1))
> 		return 0;
> 
> Well, I didn't know how to spit up something like
> "ntohs(msg->param->len) == SAP_PARAM_ID_TRANSPORT_PROTOCOL_LEN &&"
> correctly.  Any sugestion?

Use shorter define names (since they really are quite long), move the
checks into separate functions, e.g. validate_set_transport_protocol_req(),
or redo the if-statement to something like:

	if (msg->nparam != 0x01)
		return -EBADMSG;
	if (msg->param->id != SAP_PARAM_ID_TRANSPORT_PROTOCOL)
		return -EBADMSG;
	if (ntohs(msg->param->len) != SAP_PARAM_ID_TRANSPORT_PROTOCOL_LEN)
		return -EBADMSG;
        ...
	return 0;

It becomes much easier for a human to parse it that way since you get to
process the individual conditions clearly one at a time.

Take your pick ;)

Johan