Sexp conversions: ignore extra bytes

[pqwy]

The current sexp conversions marshal the entire structure, including the full underlying buffer.

This is desirable if Cstruct.t is understood as a char bigarray with a handy offset and a limit. If it's understood as a view into the underlying array, then its meaning are exactly the bytes in the array between offset and offset + length. Marshaling the entire array then leaks the details on how the view was originally obtained.

I would argue that since it's impossible to expand a cstruct, the latter is a better description of what cstructs are. Bytes beyond the stored range are not accessible through the functions in Cstruct, and should not be serialized either. If this is desired, one can easily store the buffer directly, as the type is not opaque.

This bit me when I tried to remove the custom from/to_sexp routines from tls and use the ones here; we end up storing large amounts of data, as various bits of internal state are cut-up slices of larger buffers.

[samoht]

Does this remove the dependency to camlp4? if yes, that's nice and it would be good to update _oasis as well!

[pqwy]

Hmmm. I don't think it does.

sexplib.syntax is still used for with sexp in the interface. Adding explicit sigs of the 4 conversions functions would eliminate that dep, and enable cstruct.ml/.mli to build without p4. But p4 would of course still be required for cstruct's own syntax extension.

Undecided if dropping sexplib.syntax is worth spelling out the boilerplate in the .mli...

[avsm]

Is any use of this performance sensitive? The to/from Bigarray operations are slow, but this is convenient and so bearable from my perspective...

[samoht]

I'm not sure why we expose of_sexp though. I can understand tha to_sexp is useful for debugging or error reporting, but in that case we don't care about performance.

[hannesm]

in TLS we use our own to/from_sexp implementation. This is especially useful for our tracing infrastructure, which logs all received packets and state. Running with tracing comes with a cost: sexps being allocated, written to network/disk/console, ...
I'm all for merging this PR - since marshalling a Cstruct as now might involve to marshal a lot of data not needed (everything before offset and after length) - which might contain data which should not be marshalled in the first place...

[samoht]

To be clear, I think this PR is a very good idea as well.

[pqwy]

Aaaaand before noticing @avsm's spot-on objection, I re-did the code to avoid Array1.sub (proven to be quite slow the other day) and to use cstruct's own and much faster blit_{to,from}_string (sexplib uses a loop in OCaml). The format is still the same.

Obviously, great minds think alike.

I think this is as fast as it can get. Converting buffers into strings (and then those strings into buffers if the network is involved) isn't the fastest way to transfer a large sequence of bytes somewhere, but it comes in handy in tls (primarily tracing), and could come in handy in x509 (if we want to save the original byte stream a cert came from) and nocrypto (where I can imagine dumping and reloading crypto state). I think of this as a debugging tool.

[samoht]

I'm happy to merge #44 and #45 tomorrow if no one else object.

[hannesm]

this is tested in production -- running on the pinata :) and #45 would be nice to have merged as well!

[avsm]

test case for this please