On Wed, 2019-12-18 at 10:50 +0000, David Howells wrote: > James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> 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. > The disadvantage being that the data start would likely not be > coincident with the buffer start. This would be a big issue: in several routines I allocate a buffer, fill it with ASN.1 and pass it back and the receiving routine has to free it. Now the buffer won't be freeable by the pointer I pass back because that may not be where the allocation was done. For these two reasons, I'd like to keep the work forwards behaviour. I'm reasonably ambivalent on the end pointer with a marginal preference for passing in the length remaining instead. James
