openbsc[master]: Adding compression control and final fixups

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/gerrit-log@lists.osmocom.org/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Wed Aug 24 11:17:34 UTC 2016


Patch Set 23:

(25 comments)

https://gerrit.osmocom.org/#/c/642/23/openbsc/src/gprs/gprs_sndcp_comp.c
File openbsc/src/gprs/gprs_sndcp_comp.c:

Line 40: 						      const struct
weird line breaks


Line 90: 	OSMO_ASSERT(comp_entity->nsapi_len > 0)
missing ';' and weird indent below


Line 99: 	if (comp_entity->compclass == SNDCP_XID_PROTOCOL_COMPRESSION) {
I was going to suggest the below, but I notice that data compression
might be added some day... will it?

if (comp_entity->compclass != SNDCP_XID_PROTOCOL_COMPRESSION) {
        [...]
        return NULL;
}

if (gprs_....


Line 122: 	} else {
from above theck this is never reached,
but again a placeholder for data compression... so ok


Line 151: 	OSMO_ASSERT(comp_entities);
quite a long comment on an obvious issue.

BTW, if you were to comp_free the same entities twice by accident,
this assert wouldn't hit: though talloc_free() freed the mem,
the comp_entities pointer would still be != NULL.

So usually, asserts like this don't really have any
benefit unless you *expect* calling code paths to handle
NULL pointers to indicate unset data or something.


Line 163: 			     comp_entity->entity);
hmm, not implemented, dead code, but making believe that
it does something useful?


Line 177: 	OSMO_ASSERT(comp_entities);
(one of those asserts)


Line 181: 			comp_entity_to_delete = comp_entity;
'break;' when found?


Line 195: 		     comp_entity_to_delete->entity);
(make-believe dead code again)


Line 206: 						     struct llist_head
weird line breaks


Line 215: 	OSMO_ASSERT(comp_field);
(asserts... and more below)


Line 232: struct gprs_sndcp_comp *gprs_sndcp_comp_by_comp(const struct llist_head
line breaks... and more below


Line 283: 	 * all sort of compression and is assigned fix. So we
what do you mean by "is assigned fix"?


Line 291: 			return i + 1;
whoa, why '+ 1'? I believe this will cause a lot of confusion.
In C, we should have 0-indexing, only translating to
1-indexing for humans if really necessary, e.g. in the VTY.

Now looking at the only caller gprs_sndcp_pcomp_expand()
that feeds the returned value to gprs_sndcp_pcomp_rfc1144_expand().

I notice that this returns an int while gprs_sndcp_pcomp_rfc1144_expand()
takes a uint8_t argument.

Also in gprs_sndcp_pcomp_rfc1144_expand() the returned
value seems to be expected to be exactly either 1 or 2.
Could you please explain a bit further before we decide what to do?

Or a different name like 'pcomp_type' could resolve the confusion;
maybe with an enum defining those indexes explicitly?


Line 318: 	return comp_entity->comp[comp_index - 1];
now -1 again :/
;)


https://gerrit.osmocom.org/#/c/642/23/openbsc/src/gprs/gprs_sndcp_pcomp.c
File openbsc/src/gprs/gprs_sndcp_pcomp.c:

Line 98: 	uint8_t *comp_ptr;	/* Required by slhc_compress() */
kind of superfluous comment :)


Line 141: 		break;
this is where the 1-indexed pcomp_index value is used.

What if pcomp_index < 1 or > 2? Would be good to add error
messages (or asserts if it's never to be expected) in a
'default:' case.


Line 150: 		/* Just in case the phone tags uncompressed tcp-datas
('data' is already the plural of 'datum')


Line 179: 	OSMO_ASSERT(comp_entities);
(asserts)


Line 183: 		memcpy(data_o, data_i, len);
since this happens twice, this could be a case for
'goto skip_decompression;' and having the memcpy only once
below 'return rc;' at the bottom.


Line 202: 	OSMO_ASSERT(comp_entity->algo == RFC_1144);
should these two asserts rather be error messages?
assertions terminate the program and disrupt service...


Line 231: 	OSMO_ASSERT(comp_entities);
(asserts)


Line 249: 	OSMO_ASSERT(comp_entity->algo == RFC_1144);
error messages?


https://gerrit.osmocom.org/#/c/642/23/openbsc/src/gprs/sgsn_libgtp.c
File openbsc/src/gprs/sgsn_libgtp.c:

Line 330: 	return rc_pdp;
'return 0;'
or just
'return sndcp_sn_xid_req(...)'
above


https://gerrit.osmocom.org/#/c/642/23/openbsc/src/gprs/sgsn_vty.c
File openbsc/src/gprs/sgsn_vty.c:

Line 274: 			    g_cfg->pcomp_rfc1144.s01+1, VTY_NEWLINE);
weird indenting


-- 
To view, visit https://gerrit.osmocom.org/642
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia00260dc09978844c2865957b4d43000b78b5e43
Gerrit-PatchSet: 23
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes



More information about the gerrit-log mailing list