Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose Arpv4.Make functor; require arp argument for Ipv4.connect #134

Closed
wants to merge 29 commits into from

Conversation

yomimono
Copy link
Contributor

@yomimono yomimono commented May 6, 2015

Pull ARP out of ipv4 and make it externally usable. Require explicit arp.t argument for Ipv4.connect .

Needs to be merged in tandem with mirage/mirage#400 .

@@ -133,6 +133,17 @@ Library "ethif-unix"
lwt,
lwt.unix

Library "arpv4-unix"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we easily expose a arpv4 library without unix dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch; thanks! Fixed.

Findlibname: arpv4
Modules: Arpv4
BuildDepends: tcpip,io-page,mirage-types,ipaddr,cstruct,lwt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you also need to remove Arpv4 from the list of modules in ipv4

@samoht
Copy link
Member

samoht commented May 8, 2015

# Error: Unbound module type V1_LWT.ARP`

Can you add PIN=mirage-types to .travis.yml to pin the package to its dev version? Thanks!

- OCAML_VERSION=4.02 PACKAGE=tcpip MIRAGE_MODE=unix
- OCAML_VERSION=4.01 PACKAGE=tcpip MIRAGE_MODE=xen
- OCAML_VERSION=4.02 PACKAGE=tcpip MIRAGE_MODE=unix PINS=mirage-types:https://github.com/yomimono/mirage#separate_arp
- OCAML_VERSION=4.01 PACKAGE=tcpip MIRAGE_MODE=xen PINS=mirage-types:https://github.com/yomimono/mirage#separate_arp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the url should be https://github.com/yomimono/mirage.git#separate_arp (with .git) to make opam happy.

@yomimono yomimono closed this May 12, 2015
@yomimono yomimono reopened this May 12, 2015
@hannesm
Copy link
Member

hannesm commented May 12, 2015

I really like this restructuring/separation of the arp layer.

@samoht
Copy link
Member

samoht commented May 12, 2015

Seems to compile fine but the unit tests are failing ... not sure if this is related to this PR though.

@MagnusS
Copy link
Member

MagnusS commented May 12, 2015

Hm it seems that the iperf test occasionally times out. I set the timeout to 30 seconds as the test completes in 4-5 seconds here. We could try increasing it to 60 seconds to see if it helps. Would it be possible to show the verbose test output in travis?

@samoht
Copy link
Member

samoht commented May 12, 2015

the Travis machine can be very slow when running the tests, so yes it would be nice to have an adaptative timeout. To see the tests in travis you can use --show-errors in the opam file to be verbose on all failed errors. Could be nice to be able to enable this behaviour using an env variable, feel free to open an issue on alcotest bugtracker to track that.

@MagnusS
Copy link
Member

MagnusS commented May 12, 2015

PR #137 should hopefully fix the iperf/travis test problem. The timeout is increased to two minutes and only half the data is transferred.

@yomimono
Copy link
Contributor Author

Thanks, @MagnusS ! We'll see whether that works a bit better.

@yomimono
Copy link
Contributor Author

I'm going to close this out, clean up the history, and make a new PR for the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants