Re: [PATCH 01/11] stkutil: Add SMS-PP Data Download envelope builder

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1921 bytes --]

Hi Andrew,

> > - When SMS is delivered, it is delivered in tpdu form, e.g. sc_address +
> > deliver pdu.  To obtain the sc_address, we need to decode the deliver
> > tpdu anyway.
> > - Before we know this is an SMS-PP download, we must check the dcs / pid.
> >  In order to check those, we must again decode the tpdu.
> > - SMS sc_address can actually be non-numeric.  In that case the SMS-PP
> > download should simply be dropped.
> 
> This means we have to decode the PDU, but re-encoding it is still an
> overhead having access to the TPDU.

Fair enough, I don't really feel strongly either way.  However, encoding is 
quite fast and we have to re-encode the sc_address in a different format anyway 
because of the weird sc_addr encoding rules.

> 
> > - Consistency with Send SMS proactive command.
> 
> Ok, makes sense.

One thing that comes to mind is that we might have to modify sms_encode() with 
the capability to skip encoding the sc_address field.  Otherwise our pdu will 
have some extra crap in the beginning.

> 
> >> +
> >> +/* Returns TRUE on success */
> >> +ofono_bool_t stk_pdu_from_envelope(const struct stk_envelope *envelope,
> >> +                                     unsigned char *pdu, unsigned int
> >> size, +                                     unsigned char **out_pdu,
> >> +                                     unsigned int *out_size);
> >
> > This part just looks ugly.  Can't we hide the details of char buf[512]
> > somewhere inside stk_pdu_from_envelope?
> 
> By that do you mean using a static buffer?  I'll send a patch for
> that.  I prefer a static buffer for the PDUs but thought you had
> argued against it :)

Don't recall :)  I think in this case it is ok, or you can always make the 
function re-entrant safe by making the buf argument in/out.

e.g.

char buf[512];
char *out = buf;

func(foo, bar, &out);

Regards,
-Denis