Page MenuHomeFreeBSD

add valectl to the system commands
ClosedPublic

Authored by vmaffione on Oct 24 2019, 8:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 12, 6:35 AM
Unknown Object (File)
Tue, Mar 12, 6:35 AM
Unknown Object (File)
Tue, Mar 12, 6:35 AM
Unknown Object (File)
Tue, Mar 12, 6:35 AM
Unknown Object (File)
Tue, Mar 12, 6:35 AM
Unknown Object (File)
Tue, Mar 12, 6:35 AM
Unknown Object (File)
Tue, Mar 12, 6:35 AM
Unknown Object (File)
Tue, Mar 12, 6:34 AM
Subscribers

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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 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 added a subscriber: bcr.

OK from manpages.

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

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 marked 3 inline comments as done.

Addressed reviewer comments.

(removed Makefile.depend again)

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.

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.

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.

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.