Add IPv4/6 conversions from/to Cstructs #36

nojb · 2014-10-02T13:34:33Z

nojb · 2014-10-02T13:42:18Z

Should these functions go in a module Ipaddr_cstruct so that they can be included conditionally and not add to the dependencies of the core Ipaddr ?

avsm · 2014-10-02T13:53:38Z

lib/ipaddr.ml

+    Cstruct.set_uint8 cs (2 + off) ((|~) (i >!  8));
+    Cstruct.set_uint8 cs (3 + off) ((|~) (i >!  0))
+
+  let to_cstruct i =


I would encourage not allocating cstructs in these functions, but passing in a cstruct to write into. This is because they must be allocated via Io_page in Mirage (so that the underlying page is aligned to a 4K block).

Then shall I remove the function to_cstruct altogether ? The function to_cstruct_raw works on passed-in cstruct. The only difference is that to_cstruct_raw can raise an exception.

Yeah, removing the allocator is fine, or having an alloc optional argument that can override it. So you could have

val set_cstruct : ?off:int -> t -> Cstruct.t -> unit val to_cstruct : ?(alloc:unit -> Cstruct.t) -> i -> Cstruct.t

The latter would let you pass in Io_page.(to_cstruct (make 1)) as the allocator function, and use Cstruct.create by default if not specified.

dsheets · 2014-10-02T17:51:02Z

Looks pretty good. The V6 functions are scattered around a bit (maybe due to function dependencies?) and there are no cstruct/bigstring implementations for network prefixes. Also, nothing to read ASCII representations out of cstructs.

I'm a bit hesitant to put cstruct support in the main module interface because it will be another dependency of ipaddr that requires camlp4 making removing the camlp4 dependency even harder in the future. Also, ocplib-endian, ounit, and optcomp would become implicit dependencies of ipaddr. This is kind of annoying when it's not clear to me why this library needs to require ounit or optcomp.

I support the inclusion of bigarray/bigstring functions in the primary signature but I think perhaps an Ipaddr_cstruct module which is built optionally if cstruct is available may be preferable. I've not looked at the difficulty or inefficiency of implementing such a module, though. I think including cstruct support in the main interface wouldn't be a problem if cstruct had ounit and camlp4 made into optional dependencies.

Out of curiosity, is your application packet parsing? @avsm, how do your packet parsers use cstruct+ipaddr, today (or do they)?

dsheets · 2014-10-02T17:51:48Z

Oh, also, if you could add cstruct to https://github.com/mirage/ocaml-ipaddr/blob/master/.travis-ci.sh that would placate Travis.

avsm · 2014-10-02T18:18:24Z

On 2 Oct 2014, at 18:51, David Sheets [email protected] wrote:

Looks pretty good. The V6 functions are scattered around a bit (maybe due to function dependencies?) and there are no cstruct/bigstring implementations for network prefixes. Also, nothing to read ASCII representations out of cstructs.

I'm a bit hesitant to put cstruct support in the main module interface because it will be another dependency of ipaddr that requires camlp4 making removing the camlp4 dependency even harder in the future. Also, ocplib-endian, ounit, and optcomp would become implicit dependencies of ipaddr. This is kind of annoying when it's not clear to me why this library needs to require ounit or optcomp.

Those are indeed build-time dependencies -- should be helped when we move to opam 1.2's finer grained build deps.

I support the inclusion of bigarray/bigstring functions in the primary signature but I think perhaps an Ipaddr_cstruct module which is built optionally if cstruct is available may be preferable. I've not looked at the difficulty or inefficiency of implementing such a module, though. I think including cstruct support in the main interface wouldn't be a problem if cstruct had ounit and camlp4 made into optional dependencies.

See mirage/ocaml-cstruct#35

Out of curiosity, is your application packet parsing? @avsm, how do your packet parsers use cstruct+ipaddr, today (or do they)

Nicolas is looking into adding ipv6 support to Mirage. Right now it's done manually, but could be handled via ipaddr with this pull for IPv4 as well.

-anil

nojb · 2014-10-02T22:01:58Z

Thanks @dsheets and @avsm for the comments. I have split the cstruct stuff into a different module Ipaddr_cstruct.

I needed to call a call a couple of no-ops: Ipaddr.V{4,6}.{of,to}_int32 because Ipaddr.V{4,6}.t is abstract (a good thing).
I introduced an allocator argument to V{4,6}.to_cstruct as per @avsm's suggestion. I didn't change the name to_cstruct_raw to set_cstruct to keep it in the same style as Ipaddr.to_bytes_raw, etc.
I duplicated the functions too_much, need_more inside Ipaddr_cstruct for error reporting. They could be put into a separate module (but I don't think it is worth the trouble). Maybe it would be better if the functions that deal with binary conversions (bytes and cstructs) just raised Invalid_argument instead of Parse_error ?

One slight downside of putting the cstruct stuff in its own module is that doing

open Ipaddr
open Ipaddr_cstruct

results in Ipaddr_cstruct.V{4,6} shadowing Ipaddr.V{4,6}.

Yes, @dsheets, application is packet parsing. Which is why I did not have much use for converting between ASCII and cstructs.

dsheets · 2014-10-03T08:49:06Z

It looks like @avsm is in the process of removing/optionalizing unnecessary cstruct dependencies so inclusion in the primary module is much more palatable (only ocplib-endian and optcomp are new with both also relying on camlp4 for build). As these dependencies are encapsulated by cstruct and small, I'm comfortable with first-class cstruct support in the primary module. I'm sorry to ask you to do more work, @nojb, could you please put the cstruct support back into the Ipaddr module? The shadowing in particular is very annoying.

I've been thinking about your agree with your suggestion to use Invalid_argument instead of Parse_error for raw buffer length errors. The intent of the checked function was to provide a ready-made way to try to parse a buffer as a binary address. If the user already knows they have an address in a buffer, they should use the _raw version without the length check. In this case, they don't suffer any length checks. As it's generally advised against to catch Invalid_argument because it represents a programming error, this checked function is then no more useful than an assertion where it functions as an assertion and a condition right now. I'm not certain about its utility at all and perhaps it should be removed or changed to parse ASCII strings so that _raw means bit-string representation and _exn means ASCII digits...

Finally, could Cstruct.BE.get_uint32 be used for IPv4 parsing rather than reading individual bytes?

Sorry for the vacillation and thanks very much for your contribution. :-)

mor1 · 2014-10-03T08:57:18Z

On 3 October 2014 09:49, David Sheets [email protected] wrote:

I've been thinking about your agree with your suggestion to use
Invalid_argument instead of Parse_error for raw buffer length errors. The
intent of the checked function was to provide a ready-made way to try to
parse a buffer as a binary address. If the user already knows they have an
address in a buffer, they should use the _raw version without the length
check. In this case, they don't suffer any length checks. As it's generally
advised against to catch Invalid_argument because it represents a
programming error, this checked function is then no more useful than an
assertion where it functions as an assertion and a condition right now. I'm
not certain about its utility at all and perhaps it should be removed
or changed to parse ASCII strings so that _raw means bit-string
representation and _exn means ASCII digits...

parsing ascii string representations is certainly a useful thing to do;
though _exn seems a non-obvious (indeed, orthogonal) modifier on the
function name to represent that. perhaps _str or _asc or something
instead?

Richard Mortier
[email protected]

dsheets · 2014-10-03T09:05:40Z

I agree. I think my complete interface proposal for a 3.0 would be as follows:

_raw are direct byte-representation functions into pre-allocated buffers
_new are direct byte-representation functions into callee-allocated buffers
_exn are ASCII string-representation functions that can throw
`` (nothing) are optional ASCII string-representation functions

This is a significant breaking change but I think something like this would fix the string/bytes/bigstring/cstruct representation confusion. _str is also a good suffix but I wonder if we can get away with treating the human-readable representation as 'default' because it also has an _exn case for parse errors.

I'd like to fix this problem once, correctly so we never have to think about this again.

dsheets · 2014-10-03T09:08:29Z

Oh, I forgot a fifth case for exceptional functions on ASCII reps into a specific buffer position. Perhaps this is _asc or _str.

dsheets · 2014-10-03T09:40:42Z

The prefixes for these suffixes will some of: of_string, of_bytes, of_buffer, of_cstruct, of_bigstring. The to_ versions would probably leave off the _exn case and have the (nothing) and_str(_hum?_asc?) cases throw anInvalid_argument if supplied with a buffer that is too small.

mor1 · 2014-10-03T10:25:30Z

gah. why doesn't github format as markdown when things come in from mail. now out of order...

ok-- generally agree. (and now seeing your followup.)

just to be sure i'm clear, the prefix for all these is of_cstruct? or
both of_cstruct and to_cstruct?

and then we have the following suffixes:

Ipaddr.V[46].t to/from byte-representations with caller or callee
allocated buffers
Ipaddr.V[46].t to/from ascii-representations that return t option or
throw
Ipaddr.V[46].t to/from ascii-representations into a specific caller
allocated buffer offset, that might throw

?

_raw / _new / nothing (or _opt?) / _exn as suggested seem good for
the first four.

could go with _asc (preferable to _str i think) for the fifth; or
_off?

mor1 · 2014-10-03T10:30:01Z

right. responding to @dsheets last comment:

not _hum. no. please no. ugh.

prefer _asc to _str but either ok.

proposal for to_ vs of_ versions sounds sensible. (at least close enough for it to be worth seeing an interface laid out i guess :))

nojb · 2014-10-03T10:59:58Z

I put the cstruct stuff back into Ipaddr. I am waiting to change the exception behaviour until there is a clear decision on the issue of error handling/naming/etc.

dsheets · 2014-10-03T11:07:19Z

I think we should release the cstruct support at 2.6 and design a nice, consistent interface for a 3.0. The present interface is internally consistent if not universally consistent. The task of deciding the error behaviors and which functions operate on bytes vs strings and byte-reps vs ASCII-reps is beyond the scope of a minor release. I've opened #37 to track the 3.0 redesign.

nojb · 2014-10-03T16:06:57Z

Another comment on this: the available conversion functions in Macaddr do not match those in Ipaddr. For example, there is to_bytes, but no to_bytes_raw, etc.

I wanted to add the cstruct functions to Macaddr, but I am not sure which convention I should follow: for cstructs, the *_raw functions are the most commonly used ones, but then we should add {to,of}_bytes_raw as well ?

mor1 · 2014-10-03T19:45:38Z

i guess follow the convention just discussed for ipaddr.3.0?

(or do i misunderstand the question?)

On 3 October 2014 17:06, Nicolas Ojeda Bar [email protected] wrote:

Another comment on this: the available conversion functions in Macaddr do
not match those in Ipaddr. For example, there is to_bytes, but no
to_bytes_raw, etc.

I wanted to add the cstruct functions to Macaddr, but I am not sure which
convention I should follow: for cstructs, the *_raw functions are the
most commonly used ones, but then we should add {to,of}_bytes_raw as well
?

Reply to this email directly or view it on GitHub
#36 (comment).

Richard Mortier
[email protected]

nojb · 2014-10-03T20:14:54Z

Sorry for the confusing comment. I just wanted to point out that even in the current state, the interfaces of Ipaddr and Macaddr were not consistent. In any case, I just added a couple of functions to Macaddr to convert from/to Cstructs following the current convention.

mor1 · 2014-10-03T20:16:12Z

ah right. i just meant, i guess it would make sense at some point to make
them both follow the convention just discussed :)

On 3 October 2014 21:14, Nicolas Ojeda Bar [email protected] wrote:

Sorry for the confusing comment. I just wanted to point out that even in
the current state, the interfaces of Ipaddr and Macaddr were not
consistent. In any case, I just added a couple of functions to Macaddr to
convert from/to Cstructs following the current convention.

Reply to this email directly or view it on GitHub
#36 (comment).

Richard Mortier
[email protected]

The Macaddr.t is now passed in the first position, just like in Ipaddr.V{4,6}.to_cstruct_raw.

This way it is consistent with the `of_bytes_exn' function.

avsm · 2014-11-17T18:18:42Z

@nojb @dsheets is this ready for merging?

dsheets · 2014-11-17T18:20:47Z

No. We want to clean up the variations first with some more modules/pre-applied functors. Do you need this feature imminently?

avsm · 2014-11-21T15:11:29Z

I'm merging mirage/mirage-tcpip#70 which could use this (But has temporary overrides in place to work without it)

yomimono · 2015-01-27T15:14:43Z

Any chance this will go in anytime soon? I'm working on something which will need either this or some overrides like those required for @nojb 's ipv6 patch (which are still in place).

nojb · 2015-01-27T19:10:12Z

I seem to remember that a little refactoring was necessary to make all the possible conversions fit more seamlesssly, with less code duplication and with an uniform interface (maybe in the style of ocplib-endian).

Until this is done you may be better off including the necessary functions in your code I guess :(

samoht · 2016-05-16T23:37:10Z

@dsheets any (more) opinion on that PR?

dsheets · 2016-05-17T08:58:31Z

This needs refactoring/renaming work that will likely break backward compat of the interface. Analysis/suggestions/PRs very welcome for that.

avsm · 2017-05-11T19:55:34Z

It only took 2.5 years, but mirage/ocaml-cstruct#133 has significantly reduced the dependency cone of the cstruct library. So depending on it directly from ipaddr is now feasible

for conversion to/from cstructs based on mirage#36 Co-authored-by: Nicolás Ojeda Bär <[email protected]>

avsm · 2019-07-11T08:40:39Z

It took half a decade, but fixed in #90 based on this PR

@nojb

…exp and macaddr-cstruct (4.0.0) CHANGES: * Rename the `to/from_bytes` functions to refer to `octets` instead. This distinguishes the meaning of human-readable addresses (`string`s in this library) and byte-packed representations(`octet`s in this library) from the OCaml `bytes` type that represents mutable strings. Porting code should just be a matter of renaming functions such as: - `Ipaddr.of_bytes` becomes `Ipaddr.of_octets` - `Macaddr.to_bytes` becomes `Macaddr.to_octets` * Introduce new `write_octets` functions that can write octet representations of IPv4/v6 into an existing bytestring. * Use the `domain-name` library to produce domain names from IP addresses. * Remove the `ipaddr.sexp` and `macaddr.sexp` ocamlfind subpackages and instead have `ipaddr-sexp` and `macaddr-sexp` to match the opam package names. * Add new `Ipaddr_cstruct` and `Macaddr_cstruct` libraries for conversion to/from cstructs (mirage/ocaml-ipaddr#36 @nojb @avsm)

avsm reviewed Oct 2, 2014
View reviewed changes

dsheets mentioned this pull request Oct 3, 2014

bigstring support #12

Open

Add IPv4/6 conversions from/to Cstructs

092d2eb

dsheets mentioned this pull request Oct 3, 2014

Redesign parsing/printing functions for various flavors #37

Closed

nojb added 2 commits October 3, 2014 21:09

Add Macaddr conversions from/to Cstructs

8e7a281

Fix Ipaddr cstruct tests

62015b9

nojb added 2 commits October 6, 2014 16:55

Fix arg order in `Macaddr.to_cstruct_raw'

4775e30

The Macaddr.t is now passed in the first position, just like in Ipaddr.V{4,6}.to_cstruct_raw.

Rename Macaddr.of_cstruct' to of_cstruct_exn'

7748ae7

This way it is consistent with the `of_bytes_exn' function.

dsheets force-pushed the master branch from 82c0e29 to 5f22d8f Compare November 2, 2014 15:27

hannesm mentioned this pull request Jan 23, 2015

of/to_cstruct(_exn), of/to_cstruct_raw for V4/V6. #44

Closed

hannesm mentioned this pull request Jan 5, 2019

Clarify bytestring conversion docstrings #81

Closed

avsm added a commit to avsm/ocaml-ipaddr that referenced this pull request Jul 11, 2019

Add new Ipaddr_cstruct and Macaddr_cstruct libraries

c303536

for conversion to/from cstructs based on mirage#36 Co-authored-by: Nicolás Ojeda Bär <[email protected]>

avsm mentioned this pull request Jul 11, 2019

Add new Ipaddr_cstruct and Macaddr_cstruct libraries #90

Merged

avsm closed this Jul 11, 2019

avsm mentioned this pull request Jul 12, 2019

[new release] ipaddr-cstruct, macaddr, macaddr-sexp, ipaddr, ipaddr-sexp and macaddr-cstruct (4.0.0) ocaml/opam-repository#14492

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IPv4/6 conversions from/to Cstructs #36

Add IPv4/6 conversions from/to Cstructs #36

nojb commented Oct 2, 2014

nojb commented Oct 2, 2014

avsm Oct 2, 2014

nojb Oct 2, 2014

avsm Oct 2, 2014

dsheets commented Oct 2, 2014

dsheets commented Oct 2, 2014

avsm commented Oct 2, 2014

nojb commented Oct 2, 2014

dsheets commented Oct 3, 2014

mor1 commented Oct 3, 2014

dsheets commented Oct 3, 2014

dsheets commented Oct 3, 2014

dsheets commented Oct 3, 2014

mor1 commented Oct 3, 2014

mor1 commented Oct 3, 2014

nojb commented Oct 3, 2014

dsheets commented Oct 3, 2014

nojb commented Oct 3, 2014

mor1 commented Oct 3, 2014

nojb commented Oct 3, 2014

mor1 commented Oct 3, 2014

avsm commented Nov 17, 2014

dsheets commented Nov 17, 2014

avsm commented Nov 21, 2014

yomimono commented Jan 27, 2015

nojb commented Jan 27, 2015

samoht commented May 16, 2016 •

edited

Loading

dsheets commented May 17, 2016

avsm commented May 11, 2017

avsm commented Jul 11, 2019

Add IPv4/6 conversions from/to Cstructs #36

Add IPv4/6 conversions from/to Cstructs #36

Conversation

nojb commented Oct 2, 2014

nojb commented Oct 2, 2014

avsm Oct 2, 2014

Choose a reason for hiding this comment

nojb Oct 2, 2014

Choose a reason for hiding this comment

avsm Oct 2, 2014

Choose a reason for hiding this comment

dsheets commented Oct 2, 2014

dsheets commented Oct 2, 2014

avsm commented Oct 2, 2014

nojb commented Oct 2, 2014

dsheets commented Oct 3, 2014

mor1 commented Oct 3, 2014

dsheets commented Oct 3, 2014

dsheets commented Oct 3, 2014

dsheets commented Oct 3, 2014

mor1 commented Oct 3, 2014

mor1 commented Oct 3, 2014

nojb commented Oct 3, 2014

dsheets commented Oct 3, 2014

nojb commented Oct 3, 2014

mor1 commented Oct 3, 2014

nojb commented Oct 3, 2014

mor1 commented Oct 3, 2014

avsm commented Nov 17, 2014

dsheets commented Nov 17, 2014

avsm commented Nov 21, 2014

yomimono commented Jan 27, 2015

nojb commented Jan 27, 2015

samoht commented May 16, 2016 • edited Loading

dsheets commented May 17, 2016

avsm commented May 11, 2017

avsm commented Jul 11, 2019

samoht commented May 16, 2016 •

edited

Loading