Inter-layer state / references / control buffer

[Harald Welte]

Hi Sylvain,

On Fri, Apr 30, 2010 at 12:19:33AM +0200, Sylvain Munaut wrote:
> > While this might not be ideal, it definitely keeps the details hidden
> > away from libosmocore, which is good.
> 
> FWIW, I think it's a good idea. When I had a first look at the gprs
> branch a few months ago, all those new fields required in msgb
> definitely were something I wasn't thrilled to see.

I also didn't find them particularly exciting.

> As I see it, it might not be perfect but :
>  - It does the job
>  - It's a whole lot better than the current situation
>  - It's simple to understand
> 
> One question that pops to mind is :
>  - Would allocation / freeing of those be handled by msgb routines ? I
> would think so since that'd avoid error and I don't see any loss of
> 'genericity' in doing that, but just asking.

As you can see from the current code in libosmocore, there is no
separate allocation but simply a small data array in the msgb itself,
just like the sk_buff.cb member.

> Also, would you include the various header pointer in this cb struct
> or splitted ?
> 
> Personaly, I'd still see them splitted but more something like
> "uint8_t *h[8];" and let the application define its own
> macros msgb_ns_hdr(), msgb_rsl_hdr(), msgb_gmm_hdr() that map to the
> unique 'h' pointer rather than things like
> 
>        union {
>                 unsigned char *smsh;
>                 unsigned char *llch;
>                 unsigned char *l4h;
>         };

Yes, I agree with that, too.  We can do a gradual migration, i.e. first
introduce the accessor macros (while still pointing to the named msgb members)
fields) and later removing those named msgb members.

Using accessor macros would also remove the need for those stupid typecasts all
over the code (like "struct gsm48_hdr *gh = (struct gsm48_hdr *) msgb->l3h;").
It would simply be "struct gsm48_hdr *gh = msgb_gsm48hdr(msg);" instead.

Due to time constraints I will not be working on this and rather work on
completing gprs functionality.  However, if somebody submits patches, I'm happy
to merge them.

Another stupidity that I also dislike about the current gsm_04_08_gprs.c code
is that it uses a different header (msgb->gmmh, now msgb_gmmh(msg)) than the
gsm_04_08.c code (msg->l3h).  The reason for this is that the layer stacking
in GPRS is deeper than in the GSM case.

I plan to rework it in a way that all 04.08 code (gsm or gprs) uses
msgb_gsm48h(msg) whihc resolves to msg->l3h;

The additional layers introduced by NS/BSSGP/LLC stacking will be hidden in
the cb[].  This way we can have code that works on any gsm48 msgb without
knowing if its on GSM or GPRS.  This will become important e.g. for SMS later.

> This way the msgb wouldn't show even any 'gsm' aspect and would just
> be a generic message buffer ...

yes, that is definitely one of the goals...

Cheers,
	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)