This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/OpenBSC@lists.osmocom.org/.
Harald Welte laforge at gnumonks.orgHi Alvaro,
On Tue, Apr 08, 2014 at 02:40:35PM +0200, Alvaro Neira Ayuso wrote:
> +#define oml_tlv_parse(dec, buf, len) \
> + tlv_parse(dec, &abis_nm_att_tlvdef, buf, len, 0, 0)
by doing so, you are using abis_nm_att_tlvdef, which is defining 0x01 as
NM_ATT_ABIS_CHANNEL, which is a fixed-size attribute of 3 bytes length.
That's quite something else than what you need, isn't it?
I've introduced abis_nm_att_osmo_tlvdef[] in libosmocore. Please either
use tlv_def_patch() to merge abis_nm_att_osmo_tlvdef with
abis_nm_att_tlvdef before calling tlv_parse from generic code. Or, if
you are sure that only a org.osmocom specific message can arrive, you
may use abis_nm_att_osmo_tlvdef directly.
> +static int take_reduce_power(struct msgb *msg)
this function takes the msgb pointer, even though from where it is
called in read_sock():
+ fom = (struct abis_om_fom_hdr *) msg->l3h;
+ switch (fom->msg_type) {
+ case NM_MT_SET_RADIO_ATTR:
+ rc = take_reduce_power(msg);
we already had computed the exact location of 'fom'.
Now in take_reduce_power, you do the following:
+static int take_reduce_power(struct msgb *msg)
+{
+ int recv_reduce_power;
+ struct tlv_parsed tlv_out;
+ struct gsm_bts_trx *trx = bts->c0;
+ int rc, abis_oml_hdr_len;
+
+ abis_oml_hdr_len = sizeof(struct abis_om_hdr);
+ abis_oml_hdr_len += sizeof(struct abis_om_fom_hdr);
+ abis_oml_hdr_len += sizeof(osmocom_magic) + 1;
so you basically make blind assumptions that there is na osmocom_magic
and its associated length.
However, the check_oml_msg() that you wrote accepts both the
com.ipaccess and the org.osmocom magic values. So
a) a com.ipaccess message might end up in take_reduce_power()
b) your static assumption that an osmocom magic being present is not
true.
Please properly think about such logic.
It might be best if the check_oml_message or similar would return some
information (or even store it in the msgb_cb() somewhere) which of the
vendor specific messages (if any) it has detected. Later code could
then base its processing on that distinction.
Also, as you had already resolved the fom pointer (from msg->l3,), thre
is no point in starting to calculate abis_oml_hdr_len again (and
wrongly).
Finally, regarding check_oml_msg(): It actually doedn't only check an
OML message, but it checks also the IPA headre in front. Please create
one function for one purpose. The check_oml_msg() shouldn't care about
the ipaccess_head in front. If we make it more generic, we can move it
to libosmocore as a general OML header sanity checking function. But
that's not possible right now as it depends on an ipaccess_head, which
is not present in E1 based OML.
Also,
+int add_manufacturer_id_label(struct msgb *msg, int manuf_type_id)
why use an int there? You have defined an 'enum manuf_type_id', please
use it :)
Regards,
Harald
--
- Harald Welte <laforge at gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)