Re: [PATCH v2 2/8] lib: add asn.1 encoder

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Thu, 2019-12-19 at 08:10 +0900, James Bottomley wrote:
> On Wed, 2019-12-18 at 10:50 +0000, David Howells wrote:
> > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > 
> > > +/**
> > > + * asn1_encode_octet_string - encode an ASN.1 OCTET STRING
> > > + * @data: pointer to encode at
> > > + * @data_len: bytes remaining in @data buffer
> > > + * @string: string to be encoded
> > > + * @len: length of string
> > > + *
> > > + * Note ASN.1 octet strings may contain zeros, so the length is
> > > obligatory.
> > > + */
> > > +int asn1_encode_octet_string(unsigned char **data, int
> > > *data_len,
> > > +			     const unsigned char *string, u32
> > > len)
> > 
> > I wonder if it makes more sense to pass in an end pointer and
> > return
> > the new data pointer (or an error), ie.:
> > 
> > unsigned char *asn1_encode_octet_string(unsigned char *data,
> > 				        unsigned char *data_end,
> > 					const unsigned char *string,
> > u32 len)
> 
> On the first point: people are prone to get off by one confusion on
> data_end pointers (should they point to the last byte in the buffer
> or
> one beyond).  If I look at how I use the API, I've no real use for
> either length remaining or the end pointer, so I think it makes no
> difference to the consumer, it's just stuff you have to do for the
> API.
>  If I look at the internal API use, we definitely need the length
> remaining, so I've a marginal preference for that format, but since
> it's easy to work out it is very marginal.
> 
> > Further, I wonder - does it actually make more sense to encode
> > backwards, ie. start at the end of the buffer and do the last
> > element
> > first, working towards the front.
> 
> Heh, let me ask you this: do you use a reverse polish notation
> calculator ... The problem is that it makes the ASN.1 hard to
> construct  for the API user and hard to read for the reviewer because
> of the order reversal.  Debugging is going to be a pain because
> you're going to get the output of asn1parse and have to read it
> backwards to see where the problems are.

I coded this up to see what it would look like, and I think it can all
be made to work with error pass through.  The latter is because you
want to build up sequences of

data = asn1_encode...(data, ...);
data = asn1_encode...(data, ...);
data = asn1_encode...(data, ...);

And only check for errors when you're finished.  I think the interface
looks nicer than a modifying pointer, so if you wait for the v4 patches
they'll show this new interface with the consumers.

James