Attention is currently required from: neels.
11 comments:
File include/osmocom/gsm/gsm0808_utils.h:
doesn't the DEPRECATED go before the comma?
Ack
Patch Set #1, Line 115: OSMO_DEPRECATED("use gsm0808_enc_speech_codec2() instead")
add […]
It's the first time I see such a suggestion/requirement. Usually we don't explain why something is deprecated in the compiler warnings. One can always do git-blame and see why the new API should be used. Also, IMO this is the kind of API that should have not been made public... None of the Osmocom projects are using it directly, so I would keep this as-is if you don't insist.
Patch Set #1, Line 122: OSMO_DEPRECATED("use gsm0808_enc_speech_codec_list2() instead");
same
See above.
File src/gsm/gsm0808_utils.c:
Patch Set #1, Line 267: OSMO_ASSERT(sc->type < 0x0f);
to confirm this, this assert can be dropped because the switch() above does not leave any sc->type > […]
Ack, but I believe it's better doing this change separately:
https://gerrit.osmocom.org/q/Iafb45ed66378f2c9c2480f81371e92c6d1da71a7
Patch Set #1, Line 216: Helper
(I detest comments saying "helper function" SCNR)
Not directly related to the changes I am making, so marking as resolved.
Patch Set #1, Line 224: = fals
(we could also drop this init)
https://gerrit.osmocom.org/q/Ifc6d109e27cdada0d08d2a8fc1c354f3de04f15c
(using "\a" is wrong)
This is not directly related to this patch, but could you explain why?
Patch Set #1, Line 308: return -EINVAL;
These asserts would be fine, or can even be dropped without substitute. […]
https://gerrit.osmocom.org/q/If9b4c92ace68191f5ddcc0a8a340fccbfe0f3dc0
Patch Set #1, Line 315: return rc;
I thought about this too, but IMO trying to recover the given msgb would unnecessary complicate things here. And simply shifting the tail is not enough. This recovery behavior may be acceptable for decoding API, because there can be new values and/or IEs added by 3GPP in recent revisions. But for the encoding API I see no need to tolerate any errors. If we fail to encode any of the IEs correctly, does it make sense to keep going and send the resulting PDU without a problematic IE? IMO, it does not. The usual approach for functions using this API would be simply free()ing the msgb and returning NULL. And as I said, this API should have been not made public at all...
Actually actually, why do we even return the nr of bytes added at all, amount of data is tracked in the msgb anyway. How does it look in the rest of the gsm0808 API, can we just return 0 for success, negative on error?
The rest of the gsm0808 API does not even check values returned by these functions, only the unit tests do. I will introduce some checks in the upcoming patch.
Patch Set #1, Line 439: return rc;
here the tiresome aspect, what if rc == 0. it will never happen, but still is an error. […]
Ah, indeed I need to convert rc from uint to int. Thanks for noticing this!
Patch Set #1, Line 446: return *tlv_len + 2;
same as above, only return 0 or negative?
I am afraid we don't have time for even more API changes...
To view, visit change 30574. To unsubscribe, or for help writing mail filters, visit settings.