Change in osmo-msc[master]: refactor: move RESET Osmux TLV parsing to ran_msg_a.c

[neels]

neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/19024 )

Change subject: refactor: move RESET Osmux TLV parsing to ran_msg_a.c
......................................................................


Patch Set 2:

(5 comments)

> TBH I don't see a problem with also checking for osmux support on RANAP, even if no HNGBW or whatever doesn't support it yet, it may in the future.

That's exactly why there is a supports_osmux param in ranap_is_reset_msg().

> That being said, if you think it's more convenient or there's any benefit in doing it this way for whatever reason then fine with me.

It sounds like you don't acknowledge the big problem of feeding a RANAP message to the gsm0808 TLV decoding functions. That is just plain insane, and that comes from doing message decoding in ran_peer.c where it doesn't belong at all. The "whatever reason" is absolute basic sanity of keeping completely different RAN message coding in the proper places :P

https://gerrit.osmocom.org/c/osmo-msc/+/19024/2/src/libmsc/ran_msg_a.c 
File src/libmsc/ran_msg_a.c:

https://gerrit.osmocom.org/c/osmo-msc/+/19024/2/src/libmsc/ran_msg_a.c@1283 
PS2, Line 1283: 	if (supports_osmux == NULL)
> simply check supports_osmux in bssmap_is_reset_msg()? What abnout simply returning a bool?
then i'd have to do that check twice.

bool: I want to also reflect a "don't know" case where neither support nor non-support are certain, for non-RESET related messages. Hence -1, 0, 1 rc. The caller can then simply handle the supports_osmux rc without having to convolute with the returned enum reset_msg_type.


https://gerrit.osmocom.org/c/osmo-msc/+/19024/2/src/libmsc/ran_msg_a.c@1286 
PS2, Line 1286: 	rc = tlv_parse(&tp, gsm0808_att_tlvdef(), msgb_l3(msg) + 1, msgb_l3len(msg) - 1, 0, 0);
(answering below comment here)

I was going back and forth on modifying msg->l3h.

I had already written

  const char *l3h = msgb_l2(msg) + sizeof(struct bssmap_header);
  int l3_len = msgb_l2len(msg) - sizeof(struct bssmap_header);
  rc = tlv_parse(..., l3h + 1, l3_len - 1, ...);

But dropped that again because IMHO we should rather use the proper msgb_l3len() definitions than copying the calculation code.

Also when receiving a BSSMAP message, setting l3h to the right location is the right thing to do. Since this code is called in code paths where we anyway set l3h everywhere else, it is a bit ridiculous to require a const msgb instead of using the proper msgb macros.

I also considered putting the l3h modification outside of *_is_reset_msg() but that doesn't fit well, because we're doing msg size plausibility checks below in bssmap_is_reset_msg(), and setting l3h only makes sense after those checks passed.


https://gerrit.osmocom.org/c/osmo-msc/+/19024/2/src/libmsc/ran_msg_a.c@1314 
PS2, Line 1314: 	l2->l3h = msgb_l2(l2) + sizeof(struct bssmap_header);
> Ack
(answer for this is above)


https://gerrit.osmocom.org/c/osmo-msc/+/19024/2/src/libmsc/ran_msg_a.c@1316 
PS2, Line 1316: 	switch (l2->l2h[sizeof(*bs)]) {
...and I should actually also do msgb_l3(l2)[0] here


https://gerrit.osmocom.org/c/osmo-msc/+/19024/2/src/libmsc/ran_msg_iu.c 
File src/libmsc/ran_msg_iu.c:

https://gerrit.osmocom.org/c/osmo-msc/+/19024/2/src/libmsc/ran_msg_iu.c@447 
PS2, Line 447: 	if (supports_osmux != NULL)
> You are using "if (supports_osmux)" on the other, you could use it too here.
oh you mean writing "x != NULL" vs just "x" ... well, does it matter?
(usually I'm all for 'if (x)' but here 'if (supports_osmux)' reads like we're analysing the flag, and '!= NULL' sort of hints that it is a pointer to the flag instead; so I'm changing the other one)



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

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I1ad4a3f9356216dd4bf8c48fba29fd23438810a7
Gerrit-Change-Number: 19024
Gerrit-PatchSet: 2
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Wed, 01 Jul 2020 14:51:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Comment-In-Reply-To: laforge <laforge at osmocom.org>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200701/418dd73f/attachment.htm>