[PATCH 3/3] gtp: Handle gtpv1 in gtp_update_pdp_conf() correctly

[Daniel Willmann]

Hi Harald,

On Wed, 2016-02-03 at 19:12 +0100, Harald Welte wrote:
> On Wed, Feb 03, 2016 at 06:53:31PM +0100, Daniel Willmann wrote:
> > +			if (gtpie_gettv0(ie, GTPIE_QOS_PROFILE0, 0,
> > +					 &pdp->qos_neg0,
> > +					 sizeof(pdp->qos_neg0))) {
> > +				gsn->missing++;
> > +				GTP_LOGPKG(LOGL_ERROR, peer,
> > +					    pack, len,
> > +					    "Missing conditional information field\n");
> > +				if (gsn->cb_conf)
> > +					gsn->cb_conf(type, EOF, pdp, cbp);
> > +				/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
> > +				   pdp_freepdp(pdp); */
> > +				return EOF;
> > +			}
> > +
> > +			if (gtpie_gettv2(ie, GTPIE_FL_DI, 0, &pdp->flru)) {
> > +				gsn->missing++;
> > +				GTP_LOGPKG(LOGL_ERROR, peer,
> > +					    pack, len,
> > +					    "Missing conditional information field\n");
> > +				if (gsn->cb_conf)
> > +					gsn->cb_conf(type, EOF, pdp, cbp);
> > +				/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
> > +				   pdp_freepdp(pdp); */
> > +				return EOF;
> > +			}
> 
> This looks like a bit too much of copy+paste to me.  Can't we put that

true. Unfortunately, the whole code looks like this and I tried to stick
with the "style".

> into a separate function, or if not, at least a macro that hides all the
> verbosity and the copy+paste?  Also, why is ther a commented-out section
> about cb_delete_context involved?

A function or macro will need 7 variables passed to it which makes the
whole thing quite ugly (unless the macro makes assumptions about the
local variable names). I have now put the error handling code at the end
of the function and jump to it in case of an error.

We lose the ability to infer the missing element from the line number of
the log message, but I don't think it's a big issue. The message is
still logged and wireshark can be used for debugging as well.

The section about cb_delete_context was there before (and still is in
other parts of the code). I have removed it in that function now.


Regards
Daniel

-- 
- Daniel Willmann <dwillmann at sysmocom.de>       http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte