The valectl(4) program is used to manage vale(4) switches.
Add it to the system commands so that it can be used right away.
Details
Details
- Reviewers
kevans lwhsu hrs - Group Reviewers
manpages - Commits
- rS354471: MFC r354229
rS354229: add valectl to the system commands
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Also need to modify usr.sbin/Makefile to hook to build.
BTW, currently our *ctl utilities have no - in the name. Does it make sense to keep the convention?
$ /bin/ls -1 {/sbin,/usr/{,s}bin}/*ctl /sbin/bectl /sbin/hastctl /sbin/pfctl /sbin/pfilctl /sbin/swapctl /sbin/sysctl /usr/bin/iscsictl /usr/bin/rctl /usr/bin/usbhidctl /usr/sbin/bhyvectl /usr/sbin/binmiscctl /usr/sbin/blacklistctl /usr/sbin/certctl /usr/sbin/devctl /usr/sbin/extattrctl /usr/sbin/flowctl /usr/sbin/gpioctl /usr/sbin/iovctl /usr/sbin/ip6addrctl /usr/sbin/manctl /usr/sbin/ngctl /usr/sbin/pppctl /usr/sbin/rtadvctl /usr/sbin/uhsoctl /usr/sbin/zonectl
| usr.sbin/vale-ctl/Makefile | ||
|---|---|---|
| 7 ↗ | (On Diff #63650) | Is this still needed? |
| 9 ↗ | (On Diff #63650) | Is it possible to remove this? |
Comment Actions
Thanks for the review (and sorry for the trivial flaws).
Yes, I agree to stick to the existing convention for *ctl names.
I have two more questions:
- We should add valectl to the system only if DEV_NETMAP is defined. Is it possible? I see that there are MK_* knobs in the Makefiles. How are these defined? Is there an MK_NETMAP already?
- How is Makefile.depend generated? I just copy pasted it from "usr.sbin/arp".
Comment Actions
Hmm... #1 isn't an easy thing to express, unfortunately. We'll never know in buildworld that the future kernel build will include it. MK_* knobs are explicitly defined in share/mk/src.opts.mk -- defining one for netmap and tying it in with kernel build would be non-trivial and likely kinda messy. I would recommend instead doing a runtime check for netmap in the kernel.
Do not worry about Makefile.depend; I would explicitly remove it, and bdrewery or someone will generate it as needed (though I seem to recall they're mostly defunct nowadays).
Comment Actions
Sounds good to me.
The valectl program already fails if netmap is not present, so no build-time check for MK_NETMAP or DEV_NETMAP is really needed. I was just thinking of people that want to shrink the image size as much as possible (and save a few KB in this case).
| usr.sbin/valectl/Makefile | ||
|---|---|---|
| 1 ↗ | (On Diff #63716) | This line is not required. Please remove it. |
| 6 ↗ | (On Diff #63716) | I think WARNS=3 is required to build valectl because some of type casting in valectl.c is not strictly safe. Were you able to build valectl.c? Just checking. |
| usr.sbin/valectl/valectl.4 | ||
| 28 ↗ | (On Diff #63693) | This line should also be updated: .Dt VALECTL |
| usr.sbin/valectl/Makefile | ||
|---|---|---|
| 6 ↗ | (On Diff #63716) | You are right, indeed. Without WARNS=3 (or lower) the source file does not build because of type casting. |
Comment Actions
Could you please fix the following nits? Sorry for sending comments so many times separately.
| share/man/man4/netmap.4 | ||
|---|---|---|
| 1097 ↗ | (On Diff #63734) | 4 -> 8 |
| tools/tools/netmap/lb.8 | ||
| 114 ↗ | (On Diff #63734) | 4 -> 8 |
| usr.sbin/valectl/Makefile | ||
| 4 ↗ | (On Diff #63734) | I forgot to point out this in the previous comment. This manual page should be in section 8, not 4. Please rename this with "valectl.8". |
| usr.sbin/valectl/valectl.4 | ||
| 28 ↗ | (On Diff #63734) | 4 -> 8 |
| 161 ↗ | (On Diff #63734) | This is not directly related to your patch, but I think "has been" should be "was" here. |
| usr.sbin/valectl/Makefile | ||
|---|---|---|
| 6 ↗ | (On Diff #63716) | Sorry I didn't make it clear. I was asking if it is possible to fix the casting issue and keep the original WARNS level. |
Comment Actions
No problems and thanks for the in-depth review.
| usr.sbin/valectl/Makefile | ||
|---|---|---|
| 6 ↗ | (On Diff #63716) | Oh I see. Unfortunately those warnings come from netmap_user.h, so I cannot fix them in this patch. |