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

[Holger Hans Peter Freyther]

On Wed, Aug 22, 2012 at 05:04:48PM +0200, pa...@gnumonks.org wrote:
> From: Pablo Neira Ayuso <pa...@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. :}


> +                     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 ;
>  
>       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;
>       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;
        default:
                ret = 0;
                break;
>       }
> -     return ipa_ccm;
> +     return ret;
>  }

does this read any better?


>  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);
?
> +     }
>  
>       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)