neels has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-mgw/+/32162 ) Change subject: mgcp_client: simpler error handling ...................................................................... mgcp_client: simpler error handling add_sdp(), add_lco(): - do not msgb_free() within these functions. Just return error, and move the msgb_free() to the caler. - when failing to write to the msgb, directly return error. Change-Id: I904d56f56053952c2ebbbe5dca744fafa32b333e --- M src/libosmo-mgcp-client/mgcp_client.c 1 file changed, 56 insertions(+), 54 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/62/32162/1 diff --git a/src/libosmo-mgcp-client/mgcp_client.c b/src/libosmo-mgcp-client/mgcp_client.c index 670910b..fb4c17a 100644 --- a/src/libosmo-mgcp-client/mgcp_client.c +++ b/src/libosmo-mgcp-client/mgcp_client.c @@ -1131,47 +1131,43 @@ #define MGCP_AUEP_MANDATORY (MGCP_MSG_PRESENCE_ENDPOINT) #define MGCP_RSIP_MANDATORY 0 /* none */ +#define MSGB_PRINTF_OR_RET(msg, FMT, ARGS...) \ + if (msgb_printf(msg, FMT, ##ARGS) != 0) { \ + LOGP(DLMGCP, LOGL_ERROR, "Message buffer to small, can not generate MGCP/SDP message\n"); \ + return -ENOBUFS; \ + } + /* Helper function for mgcp_msg_gen(): Add LCO information to MGCP message */ static int add_lco(struct msgb *msg, struct mgcp_msg *mgcp_msg) { unsigned int i; - int rc = 0; const char *codec; unsigned int pt; - rc |= msgb_printf(msg, "L:"); + MSGB_PRINTF_OR_RET(msg, "L:"); if (mgcp_msg->ptime) - rc |= msgb_printf(msg, " p:%u,", mgcp_msg->ptime); + MSGB_PRINTF_OR_RET(msg, " p:%u,", mgcp_msg->ptime); if (mgcp_msg->codecs_len) { - rc |= msgb_printf(msg, " a:"); + MSGB_PRINTF_OR_RET(msg, " a:"); for (i = 0; i < mgcp_msg->codecs_len; i++) { pt = mgcp_msg->codecs[i]; codec = get_value_string_or_null(osmo_mgcpc_codec_names, pt); /* Note: Use codec descriptors from enum mgcp_codecs * in mgcp_client only! */ - if (!codec) { - msgb_free(msg); + if (!codec) return -EINVAL; - } - rc |= msgb_printf(msg, "%s", extract_codec_name(codec)); + MSGB_PRINTF_OR_RET(msg, "%s", extract_codec_name(codec)); if (i < mgcp_msg->codecs_len - 1) - rc |= msgb_printf(msg, ";"); + MSGB_PRINTF_OR_RET(msg, ";"); } - rc |= msgb_printf(msg, ","); + MSGB_PRINTF_OR_RET(msg, ","); } - rc |= msgb_printf(msg, " nt:IN\r\n"); - - if (rc != 0) { - LOGP(DLMGCP, LOGL_ERROR, - "message buffer to small, can not generate MGCP message (LCO)\n"); - msgb_free(msg); - return -ENOBUFS; - } + MSGB_PRINTF_OR_RET(msg, " nt:IN\r\n"); return 0; } @@ -1179,43 +1175,37 @@ static int add_sdp(struct msgb *msg, struct mgcp_msg *mgcp_msg, struct mgcp_client *mgcp) { unsigned int i; - int rc = 0; char local_ip[INET6_ADDRSTRLEN]; int local_ip_family, audio_ip_family; const char *codec; unsigned int pt; /* Add separator to mark the beginning of the SDP block */ - rc |= msgb_printf(msg, "\r\n"); + MSGB_PRINTF_OR_RET(msg, "\r\n"); /* Add SDP protocol version */ - rc |= msgb_printf(msg, "v=0\r\n"); + MSGB_PRINTF_OR_RET(msg, "v=0\r\n"); /* Determine local IP-Address */ if (osmo_sock_local_ip(local_ip, mgcp->actual.remote_addr) < 0) { LOGPMGW(mgcp, LOGL_ERROR, "Could not determine local IP-Address!\n"); - msgb_free(msg); return -EINVAL; } local_ip_family = osmo_ip_str_type(local_ip); - if (local_ip_family == AF_UNSPEC) { - msgb_free(msg); + if (local_ip_family == AF_UNSPEC) return -EINVAL; - } audio_ip_family = osmo_ip_str_type(mgcp_msg->audio_ip); - if (audio_ip_family == AF_UNSPEC) { - msgb_free(msg); + if (audio_ip_family == AF_UNSPEC) return -EINVAL; - } /* Add owner/creator (SDP) */ - rc |= msgb_printf(msg, "o=- %x 23 IN IP%c %s\r\n", mgcp_msg->call_id, + MSGB_PRINTF_OR_RET(msg, "o=- %x 23 IN IP%c %s\r\n", mgcp_msg->call_id, local_ip_family == AF_INET6 ? '6' : '4', local_ip); /* Add session name (none) */ - rc |= msgb_printf(msg, "s=-\r\n"); + MSGB_PRINTF_OR_RET(msg, "s=-\r\n"); /* Add RTP address and port */ if (mgcp_msg->audio_port == 0) { @@ -1230,20 +1220,20 @@ msgb_free(msg); return -EINVAL; } - rc |= msgb_printf(msg, "c=IN IP%c %s\r\n", + MSGB_PRINTF_OR_RET(msg, "c=IN IP%c %s\r\n", audio_ip_family == AF_INET6 ? '6' : '4', mgcp_msg->audio_ip); /* Add time description, active time (SDP) */ - rc |= msgb_printf(msg, "t=0 0\r\n"); + MSGB_PRINTF_OR_RET(msg, "t=0 0\r\n"); - rc |= msgb_printf(msg, "m=audio %u RTP/AVP", mgcp_msg->audio_port); + MSGB_PRINTF_OR_RET(msg, "m=audio %u RTP/AVP", mgcp_msg->audio_port); for (i = 0; i < mgcp_msg->codecs_len; i++) { pt = map_codec_to_pt(mgcp_msg->ptmap, mgcp_msg->ptmap_len, mgcp_msg->codecs[i]); - rc |= msgb_printf(msg, " %u", pt); + MSGB_PRINTF_OR_RET(msg, " %u", pt); } - rc |= msgb_printf(msg, "\r\n"); + MSGB_PRINTF_OR_RET(msg, "\r\n"); /* Add optional codec parameters (fmtp) */ if (mgcp_msg->param_present) { @@ -1253,9 +1243,9 @@ continue; pt = map_codec_to_pt(mgcp_msg->ptmap, mgcp_msg->ptmap_len, mgcp_msg->codecs[i]); if (mgcp_msg->param.amr_octet_aligned_present && mgcp_msg->param.amr_octet_aligned) - rc |= msgb_printf(msg, "a=fmtp:%u octet-align=1\r\n", pt); + MSGB_PRINTF_OR_RET(msg, "a=fmtp:%u octet-align=1\r\n", pt); else if (mgcp_msg->param.amr_octet_aligned_present && !mgcp_msg->param.amr_octet_aligned) - rc |= msgb_printf(msg, "a=fmtp:%u octet-align=0\r\n", pt); + MSGB_PRINTF_OR_RET(msg, "a=fmtp:%u octet-align=0\r\n", pt); } } @@ -1275,18 +1265,12 @@ return -EINVAL; } - rc |= msgb_printf(msg, "a=rtpmap:%u %s\r\n", pt, codec); + MSGB_PRINTF_OR_RET(msg, "a=rtpmap:%u %s\r\n", pt, codec); } } if (mgcp_msg->ptime) - rc |= msgb_printf(msg, "a=ptime:%u\r\n", mgcp_msg->ptime); - - if (rc != 0) { - LOGPMGW(mgcp, LOGL_ERROR, "Message buffer to small, can not generate MGCP message (SDP)\n"); - msgb_free(msg); - return -ENOBUFS; - } + MSGB_PRINTF_OR_RET(msg, "a=ptime:%u\r\n", mgcp_msg->ptime); return 0; } @@ -1311,23 +1295,23 @@ switch (mgcp_msg->verb) { case MGCP_VERB_CRCX: mandatory_mask = MGCP_CRCX_MANDATORY; - rc |= msgb_printf(msg, "CRCX %u", trans_id); + MSGB_PRINTF_OR_RET(msg, "CRCX %u", trans_id); break; case MGCP_VERB_MDCX: mandatory_mask = MGCP_MDCX_MANDATORY; - rc |= msgb_printf(msg, "MDCX %u", trans_id); + MSGB_PRINTF_OR_RET(msg, "MDCX %u", trans_id); break; case MGCP_VERB_DLCX: mandatory_mask = MGCP_DLCX_MANDATORY; - rc |= msgb_printf(msg, "DLCX %u", trans_id); + MSGB_PRINTF_OR_RET(msg, "DLCX %u", trans_id); break; case MGCP_VERB_AUEP: mandatory_mask = MGCP_AUEP_MANDATORY; - rc |= msgb_printf(msg, "AUEP %u", trans_id); + MSGB_PRINTF_OR_RET(msg, "AUEP %u", trans_id); break; case MGCP_VERB_RSIP: mandatory_mask = MGCP_RSIP_MANDATORY; - rc |= msgb_printf(msg, "RSIP %u", trans_id); + MSGB_PRINTF_OR_RET(msg, "RSIP %u", trans_id); break; default: LOGPMGW(mgcp, LOGL_ERROR, "Invalid command verb, can not generate MGCP message\n"); @@ -1359,15 +1343,15 @@ return NULL; } - rc |= msgb_printf(msg, " %s", mgcp_msg->endpoint); + MSGB_PRINTF_OR_RET(msg, " %s", mgcp_msg->endpoint); } /* Add protocol version */ - rc |= msgb_printf(msg, " MGCP 1.0\r\n"); + MSGB_PRINTF_OR_RET(msg, " MGCP 1.0\r\n"); /* Add call id */ if (mgcp_msg->presence & MGCP_MSG_PRESENCE_CALL_ID) - rc |= msgb_printf(msg, "C: %x\r\n", mgcp_msg->call_id); + MSGB_PRINTF_OR_RET(msg, "C: %x\r\n", mgcp_msg->call_id); /* Add connection id */ if (mgcp_msg->presence & MGCP_MSG_PRESENCE_CONN_ID) { @@ -1376,7 +1360,7 @@ msgb_free(msg); return NULL; } - rc |= msgb_printf(msg, "I: %s\r\n", mgcp_msg->conn_id); + MSGB_PRINTF_OR_RET(msg, "I: %s\r\n", mgcp_msg->conn_id); } /* Using SDP makes sense when a valid IP/Port combination is specifiec, @@ -1391,6 +1375,7 @@ || mgcp_msg->verb == MGCP_VERB_MDCX)) { if (add_lco(msg, mgcp_msg) < 0) { LOGPMGW(mgcp, LOGL_ERROR, "Failed to add LCO, can not generate MGCP message\n"); + msgb_free(msg); return NULL; } } @@ -1429,6 +1414,7 @@ || mgcp_msg->verb == MGCP_VERB_MDCX)) { if (add_sdp(msg, mgcp_msg, mgcp) < 0) { LOGPMGW(mgcp, LOGL_ERROR, "Failed to add SDP, can not generate MGCP message\n"); + msgb_free(msg); return NULL; } } -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32162 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I904d56f56053952c2ebbbe5dca744fafa32b333e Gerrit-Change-Number: 32162 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr@sysmocom.de> Gerrit-MessageType: newchange