Closed pjkundert closed 7 months ago
Hi!
First, I'd like to apologize — the notification email for your pull request got buried under a ton of other things, and I'm only looking at your changes now. 1 month, phew, that sucks. Really sorry.
I'm willing to merge the new argument parsing, and the route option. However, do you think the verbose mode could be left to #11?
Also, I'm OK with changing the various conditionals and quoting, put please-please can you put that in a different PR? (That will make it much easier for me to review.)
In any case, thanks a lot for your contribution, and I hope that you're not too unhappy about my slow answer :-(
Best,
I've rebased this pull request on to current master, and augmented it. o pull out error, warning and log messages, and emit them to stderr o Cleaned up code to deduce container PID, using either raw cgroups or docker
Corrected some further issues with arping; checking for the availability of the command is useless, because the container (usually) will have a different set of commands available wtihin it (and we cannot check).
So, make it optional with -a|--arp, and warn on command failure. Also, since there are (at least) two versions of arping in widespread use, made a documentation note in the code to indicate what version is necessary, for those that are interested.
Also added -x|--trace to enable bash command tracing; very useful for working out failures. Incidentally, "set -e" is widely considered to be quite fragile, but I think I've rooted out most of the unrecognized failures due to its use...
Sorry again — since your PR is bigger than others, it always went to the bottom of the stack, as I don't get a lot of time to work on pipework :-/
Some of those changes have been merged separately already.
If you are still willing to work on this PR, could you break it down in multiple parts?
Specifically:
Note regarding arping: with ip netns exec
, we are executing within the namespace of the container, but using the binaries in our namespace, so it makes sense to check for arping anyway :-)
The parsing of arguments has been improved, adding support for long- and short-form options.
New -v|--verbose option allows pipework to print a play-by-play of argument parsing and what it is doing, to assist in diagnosis when things go unexpectedly.
The new -r|--route option allows specifying one or more new routes in the container using the new interface. This is useful, for example, to set up multicast to use the new interface, eg. --route 224.0.0.0/4
Also cleaned up some of the conditionals and variable quoting.