Change in ...osmo-remsim[master]: rspro_dec_msg: Simplify msgb ownership handling

[laforge]

laforge has submitted this change and it was merged. ( https://gerrit.osmocom.org/c/osmo-remsim/+/15507 )

Change subject: rspro_dec_msg: Simplify msgb ownership handling
......................................................................

rspro_dec_msg: Simplify msgb ownership handling

Initially it seemed like a good idea that rspro_dec_msg() takes care
of freeing the input msgb when generating a new decoded output
structure.  However, in reality this made the implementation of
every caller more complicated, as it had to treat messages going into
rspro_dec_msg() differently than messages going elsewhere.

Adding to that, not every caller got it right, and the comments were
disagreeing about what happens to msgb ownership in erroneous cases.

Change-Id: I55d5d61922053fd94e2b5a8cdf0d799b29feec98
---
M src/remsim_client.c
M src/rspro_client_fsm.c
M src/rspro_util.c
M src/simtrace2-remsim_client.c
4 files changed, 6 insertions(+), 8 deletions(-)

Approvals:
  Jenkins Builder: Verified
  laforge: Looks good to me, approved



diff --git a/src/remsim_client.c b/src/remsim_client.c
index e73fcc4..0bfc0a4 100644
--- a/src/remsim_client.c
+++ b/src/remsim_client.c
@@ -90,7 +90,6 @@
 		default:
 			break;
 		}
-		msgb_free(msg);
 		break;
 	case IPAC_PROTO_OSMO:
 		if (!he || msgb_l2len(msg) < sizeof(*he))
@@ -109,6 +108,7 @@
 		goto invalid;
 	}
 
+	msgb_free(msg);
 	return rc;
 
 invalid:
diff --git a/src/rspro_client_fsm.c b/src/rspro_client_fsm.c
index d487f32..45cd08e 100644
--- a/src/rspro_client_fsm.c
+++ b/src/rspro_client_fsm.c
@@ -133,7 +133,6 @@
 		default:
 			break;
 		}
-		msgb_free(msg);
 		break;
 	case IPAC_PROTO_OSMO:
 		if (!he || msgb_l2len(msg) < sizeof(*he))
@@ -146,7 +145,7 @@
 			 * and unsuccessful cases */
 			pdu = rspro_dec_msg(msg);
 			if (!pdu)
-				goto invalid;
+				break;
 			rc = srvc->handle_rx(srvc, pdu);
 			ASN_STRUCT_FREE(asn_DEF_RsproPDU, pdu);
 			break;
@@ -157,6 +156,8 @@
 	default:
 		goto invalid;
 	}
+
+	msgb_free(msg);
 	return rc;
 
 invalid:
diff --git a/src/rspro_util.c b/src/rspro_util.c
index 1e5ce99..5c78b60 100644
--- a/src/rspro_util.c
+++ b/src/rspro_util.c
@@ -80,7 +80,7 @@
 	return msg;
 }
 
-/* consumes 'msg' _if_ it is successful */
+/* caller must make sure to free msg */
 RsproPDU_t *rspro_dec_msg(struct msgb *msg)
 {
 	RsproPDU_t *pdu = NULL;
@@ -91,12 +91,9 @@
 	if (rval.code != RC_OK) {
 		fprintf(stderr, "Failed to decode: %d. Consumed %lu of %u bytes\n",
 			rval.code, rval.consumed, msgb_length(msg));
-		msgb_free(msg);
 		return NULL;
 	}
 
-	msgb_free(msg);
-
 	return pdu;
 }
 
diff --git a/src/simtrace2-remsim_client.c b/src/simtrace2-remsim_client.c
index 094773f..04aa0c6 100644
--- a/src/simtrace2-remsim_client.c
+++ b/src/simtrace2-remsim_client.c
@@ -578,7 +578,6 @@
 
 static int bankd_handle_msg(struct bankd_client *bc, struct msgb *msg)
 {
-	/* rspro_dec_msg takes ownership of msgb and talloc_free()s it in successful and unsuccessful case */
 	RsproPDU_t *pdu = rspro_dec_msg(msg);
 	if (!pdu) {
 		LOGPFSML(bc->bankd_fi, LOGL_ERROR, "Error decoding PDU\n");
@@ -628,6 +627,7 @@
 	LOGPFSML(bc->bankd_fi, LOGL_DEBUG, "Received RSPRO %s\n", msgb_hexdump(msg));
 
 	rc = bankd_handle_msg(bc, msg);
+	msgb_free(msg);
 	return rc;
 
 invalid:

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-remsim/+/15507
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-remsim
Gerrit-Branch: master
Gerrit-Change-Id: I55d5d61922053fd94e2b5a8cdf0d799b29feec98
Gerrit-Change-Number: 15507
Gerrit-PatchSet: 1
Gerrit-Owner: laforge <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at gnumonks.org>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190912/3536da45/attachment.htm>