openbsc[master]: Adding SNDCP-XID encoder / decoder

[Neels Hofmeyr]

Patch Set 8: Code-Review-1

(16 comments)

https://gerrit.osmocom.org/#/c/641/8//COMMIT_MSG
Commit Message:

Line 7: Adding SNDCP-XID encoder / decoder
"Add SNDCP-XID encoder / decoder and unit test"


https://gerrit.osmocom.org/#/c/641/8/openbsc/src/gprs/gprs_sndcp_xid.c
File openbsc/src/gprs/gprs_sndcp_xid.c:

Line 1635: 			return NULL;
missing free of comp_fields in case of error


Line 1641: 			return NULL;
Though this also looks like a missing free of comp_fields in case of error, it 
actually isn't, because gprs_sndcp_decode_xid() does the talloc_free on
comp_fields.

While this is technically correct, I would prefer if you would always free 
comp_fields in this function instead of delegating to each and every error case
of called functions.

So, let the other return error, and here have e.g.:

gprs_sndcp_parse_xid()
{
        rc = gprs_sndcp_foo(...);
        if (rc)
                goto error_free_comp_fields

        rc = gprs_sndcp_bar(...);
        if (rc)
                goto error_free_comp_fields

        [...]

        return comp_fields;

error_free_comp_fields:
        talloc_free(comp_fields);
        return NULL;
}
       
If you grep for goto, you will find numerous instances of this kind
of error handling ... and this is the *only* way we use goto :)
https://www.xkcd.com/292/


Line 1646: 			return NULL;
same


Line 1652: 			return NULL;
same


Line 1660: {
personally, I would just call talloc_free(comp_fields) and drop this function. The callers generally can expect comp_fields to never be null.


Line 1868: 		LOGP(DSNDCP, logl, "\n");
really log a blank line? :)


https://gerrit.osmocom.org/#/c/641/8/openbsc/tests/sgsn/Makefile.am
File openbsc/tests/sgsn/Makefile.am:

Line 34: 	$(top_builddir)/src/gprs/gprs_sndcp_xid.o \
sgsn-test is not using this, no need to add anything


Line 42: 	-lgtp -lrt -lm
this is not needed either


https://gerrit.osmocom.org/#/c/641/8/openbsc/tests/testsuite.at
File openbsc/tests/testsuite.at:

Line 136: AT_CHECK([$abs_top_builddir/tests/xid_sndcp/sndcp_xid_test], [], [expout], [ignore])
ah, so you decided against checking stderr.


Line 138: 
rather not add a blank line in the end


https://gerrit.osmocom.org/#/c/641/8/openbsc/tests/xid_sndcp/Makefile.am
File openbsc/tests/xid_sndcp/Makefile.am:

Line 20: 
I know it's nitpicking, but generally we're pretty tight on whitespace in osmocom code ... drop the blank lines at EOF.


https://gerrit.osmocom.org/#/c/641/8/openbsc/tests/xid_sndcp/sndcp_xid_test.c
File openbsc/tests/xid_sndcp/sndcp_xid_test.c:

Line 54: 	gprs_sndcp_dump_comp_fields(comp_fields, DSNDCP);
would be nice to check this dump in experr?


Line 60: 	printf("Rencoded: %s\n", osmo_hexdump_nospc(xid_r, rc));
"Reencoded" / "Re-encoded"?


Line 240: 	gprs_sndcp_dump_comp_fields(comp_fields_dec, DSNDCP);
again check this in experr?


Line 276: }
you need to add a stub for bssgp_prim_cb like in sgsn_test.c:

/* stubs */
struct osmo_prim_hdr;
int bssgp_prim_cb(struct osmo_prim_hdr *oph, void *ctx)
{
        abort();
}


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If2d63fe2550864cafef3156b1dc0629037c49c1e
Gerrit-PatchSet: 8
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