[PATCH 06/11] ipaccess: more robust error handling in the send path

[Pablo Neira Ayuso]

On Thu, Aug 23, 2012 at 09:19:17AM +0200, Holger Hans Peter Freyther wrote:
> On Wed, Aug 22, 2012 at 05:04:48PM +0200, pablo at gnumonks.org wrote:
> > From: Pablo Neira Ayuso <pablo at gnumonks.org>
> > 
> > In case of problems to transmit message, spot an error and
> > close the socket to force a re-establishment.
> > 
> > This also modifies ipa_client_read to close the socket in
> > case that our callback reports an error.
> 
> this somehow doesn't read right. Sorry to be picky here. :}

No problem, I'll fix the description.

> 
> > +			LOGP(DLINP, LOGL_ERROR, "connection closed with "
> > +						"server\n");
> 
> I try to have full sentences in the log message. This means it starts
> with a capital letter and ends with a full stop.
> 
> 
> >  	int ret = 0;
> 	int ret = 1 ;

This will not help, ret is overwritten by ipaccess_send_*, I need to
reload it anyway for the PING and ID_ACK cases after the send().

I have changed this to:

int ret;

And below, inlined the changes I've done for the new patch version:

> >  
> >  	switch (msg_type) {
> >  	case IPAC_MSGT_PING:
> > -		ipa_ccm = 1;
> >  		ret = ipaccess_send_pong(bfd->fd);
> > +		if (ret < 0) {
> > +			LOGP(DLINP, LOGL_ERROR, "cannot send PING "
> > +			     "message. Reason: %s\n", strerror(errno));
                             break;

> 		}

                ret = 1;

>
> 
> >  		break;
> >  	case IPAC_MSGT_PONG:
> >  		DEBUGP(DLMI, "PONG!\n");
> >  		break;
> >  	case IPAC_MSGT_ID_ACK:
> >  		DEBUGP(DLMI, "ID_ACK? -> ACK!\n");
> > -		ipa_ccm = 1;
> >  		ret = ipaccess_send_id_ack(bfd->fd);
> > +		if (ret < 0) {
> > +			LOGP(DLINP, LOGL_ERROR, "cannot send ID_ACK "
> > +			     "message. Reason: %s\n", strerror(errno));
                        break;
> 		}

                ret = 1;

> >  		break;
> 	default:
> 		ret = 0;
> 		break;
> >  	}
> > -	return ipa_ccm;
> > +	return ret;
> >  }
> 
> does this read any better?

If you refer to remove ipa_ccm and to use ret instead, well, it's just
that I have merged ret and ipa_ccm, and ipa_ccm = send(...) seems ugly
to me.

If you refer to the entire function, yes I think it reads better now.

> >  static int ipaccess_drop(struct osmo_fd *bfd)
> > @@ -269,8 +280,15 @@ static int ipaccess_rcvmsg(struct e1inp_line *line, struct msgb *msg,
> >  	int len, ret;
> >  
> >  	/* Handle IPA PING, PONG and ID_ACK messages. */
> > -	if (ipaccess_rcvmsg_base(msg, bfd))
> > +	ret = ipaccess_rcvmsg_base(msg, bfd);
> > +	switch(ret) {
> > +	case -1:
> > +		/* error in IPA control message handling */
> > +		goto err;
> > +	case 1:
> > +		/* this is an IPA control message, skip further processing */
> >  		return 0;
> 	default:
> 		WARN_ON(ret);
> ?

Added case 0 (this is not an IPA control message) and default case
using LOGP to report an error.

> > +	}
> >  
> >  	switch (msg_type) {
> >  	case IPAC_MSGT_ID_RESP:
> > @@ -517,6 +535,16 @@ static int __handle_ts1_write(struct osmo_fd *bfd, struct e1inp_line *line)
> >  	DEBUGP(DLMI, "TX %u: %s\n", ts_nr, osmo_hexdump(msg->l2h, msgb_l2len(msg)));
> >  
> >  	ret = send(bfd->fd, msg->data, msg->len, 0);
> > +	if (ret < 0) {
> 	if (ret != msg->len)

I've changed all send callers to check for ret != msg->len. Note that
if we ever support TCP segmentation (TCP_NODELAY unset), then we'll
have to relax that checking (although I don't think we'll ever do,
unless for the ipaccess BTS).

Thanks for you review Holger.