Page MenuHomeFreeBSD

add valectl to the system commands
ClosedPublic

Authored by vmaffione on Oct 24 2019, 8:19 PM.

Details

Summary

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vmaffione created this revision.Oct 24 2019, 8:19 PM
lwhsu added a comment.Oct 25 2019, 8:25 PM

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?

vmaffione updated this revision to Diff 63693.Oct 26 2019, 12:07 PM
vmaffione marked 2 inline comments as done.
vmaffione edited the summary of this revision. (Show Details)

Addressed reviewers' comments

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:

  1. 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?
  2. How is Makefile.depend generated? I just copy pasted it from "usr.sbin/arp".
vmaffione retitled this revision from add vale-ctl to the system commands to add valectl to the system commands.Oct 26 2019, 12:10 PM
bcr accepted this revision as: manpages.Oct 26 2019, 8:43 PM
bcr added a subscriber: bcr.

OK from manpages.

lwhsu added a reviewer: hrs.Oct 26 2019, 8:47 PM

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:

  1. 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?
  2. How is Makefile.depend generated? I just copy pasted it from "usr.sbin/arp".

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).

vmaffione updated this revision to Diff 63716.Oct 27 2019, 11:39 AM

Removed Makefile.depend

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).

hrs requested changes to this revision.Oct 28 2019, 6:32 PM
hrs added inline comments.
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

This revision now requires changes to proceed.Oct 28 2019, 6:32 PM
vmaffione updated this revision to Diff 63733.Oct 28 2019, 9:03 PM
vmaffione marked 3 inline comments as done.

Addressed reviewer comments.

vmaffione updated this revision to Diff 63734.Oct 28 2019, 9:05 PM

(removed Makefile.depend again)

vmaffione added inline comments.Oct 28 2019, 9:31 PM
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.
Thanks.

hrs added a comment.Oct 29 2019, 3:33 AM

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.

lwhsu added inline comments.Oct 29 2019, 3:38 AM
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.

vmaffione updated this revision to Diff 63761.Oct 29 2019, 5:40 PM
vmaffione marked 6 inline comments as done.

Addressed more comments.
valectl.4 renamed to valectl.8

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.

hrs accepted this revision.Oct 31 2019, 4:59 AM

Looks good to me.

This revision is now accepted and ready to land.Oct 31 2019, 4:59 AM
This revision was automatically updated to reflect the committed changes.