mirage / mirage-vnetif

Virtual network interface and software bridge for Mirage
ISC License
16 stars 13 forks source link

Add optional size_limit to Vnetif.create; assert to writes #16

Closed yomimono closed 7 years ago

yomimono commented 7 years ago

Useful for testing that overly-large packets are not created by the stack. (See https://github.com/yomimono/mirage-tcpip/tree/test-mtus for an example.)

I can easily remove the Logs dependency if you'd prefer not to have it.

MagnusS commented 7 years ago

Thanks! But couldn't this be implemented as a custom backend for the test without changing the base backend? E.g. as in https://github.com/mirage/mirage-tcpip/blob/master/lib_test/vnetif_backends.ml#L79

yomimono commented 7 years ago

That was my initial approach, but it doesn't work for the case where you want to have two stacks representing different MTUs on the same backend. Of course you don't really want them on the same backend, you just want them to be able to reach each other, but I don't know of a way to do that in vnetif other than putting them on the same bridge.

If multiple backends could communicate, I don't think this patch would be necessary.

yomimono commented 7 years ago

I should've mentioned that I did add an MTU-enforcing backend as part of the PR in tcpip: https://github.com/mirage/mirage-tcpip/pull/313/files#diff-6c765e91e585d664129fdfb013c99bb3R25 , but for a couple of tests that isn't enough for the reason I edited in above.

MagnusS commented 7 years ago

Yes - seems reasonable to add this here, and we can always change it if we come up with a better way later.

I don't mind the Logs dependency if it doesn't add overhead to each packet sent when debug is off

yomimono commented 7 years ago

When debug is off, the calls to Logs.debug don't even do message formatting (hence the slightly weird Logs.debug @@ fun f -> f "some format string" format_string_arg pattern), so the overhead should be minimal/nonexistent assuming I've understood http://erratique.ch/software/logs/doc/Logs.html correctly.

MagnusS commented 7 years ago

LGTM