sys/net/debugnet.c
41 ↗	(On Diff #63479)	What's this needed for?
sys/netinet/netdump/netdump_client.c
412–425 ↗	(On Diff #63479)	Isn't this now referencing an undefined variable in the `!VIMAGE` case? Even if the macro is compiled out (I did not check), it still seems misleading.

tuexen added inline comments.Oct 19 2019, 7:04 PM

sys/net/debugnet.c

41 ↗

(On Diff #63479)

When compiling a kernel without VIMAGE, I get:

tuexen@head:~/head/sys/amd64/compile/TCP % make
cc -c -O2 -pipe -fno-strict-aliasing  -g -nostdinc  -I. -I../../.. -I../../../contrib/ck/include -I../../../contrib/libfdt -D_KERNEL -DHAVE_KERNEL_OPTION_HEADERS -include opt_global.h -fsanitize-coverage=trace-pc,trace-cmp   -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -MD  -MF.depend.debugnet.o -MTdebugnet.o -fdebug-prefix-map=./machine=/usr/home/tuexen/head/sys/amd64/include -fdebug-prefix-map=./x86=/usr/home/tuexen/head/sys/x86/include -mcmodel=kernel -mno-red-zone -mno-mmx -mno-sse -msoft-float  -fno-asynchronous-unwind-tables -ffreestanding -fwrapv -fstack-protector -gdwarf-2 -Wall -Wredundant-decls -Wnested-externs -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wcast-qual -Wundef -Wno-pointer-sign -D__printf__=__freebsd_kprintf__ -Wmissing-include-dirs -fdiagnostics-show-option -Wno-unknown-pragmas -Wno-error-tautological-compare -Wno-error-empty-body -Wno-error-parentheses-equality -Wno-error-unused-function -Wno-error-pointer-sign -Wno-error-shift-negative-value -Wno-address-of-packed-member -Wno-format-zero-length   -mno-aes -mno-avx  -std=iso9899:1999 -Werror  ../../../net/debugnet.c
../../../net/debugnet.c:854:21: error: implicit declaration of function 'EVENTHANDLER_REGISTER' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        dn_attach_cookie = EVENTHANDLER_REGISTER(ifnet_link_event,
                           ^
../../../net/debugnet.c:854:43: error: use of undeclared identifier 'ifnet_link_event'
        dn_attach_cookie = EVENTHANDLER_REGISTER(ifnet_link_event,
                                                 ^
../../../net/debugnet.c:855:28: error: use of undeclared identifier 'EVENTHANDLER_PRI_ANY'
            dn_ifnet_event, NULL, EVENTHANDLER_PRI_ANY);
                                  ^
3 errors generated.
*** Error code 1

Stop.
make: stopped in /usr/home/tuexen/head/sys/amd64/compile/TCP

sys/netinet/netdump/netdump_client.c

412–425 ↗

(On Diff #63479)

The variable os not even declared in that case . This compiles, because the VNET macros are empty in that case.
If you prefer, the #endif can be moved before the line

ifp = ifunit_ref(conf->kda_iface);

cem added inline comments.Oct 19 2019, 7:10 PM

sys/net/debugnet.c
41 ↗	(On Diff #63479)	Thanks!
sys/netinet/netdump/netdump_client.c
412–425 ↗	(On Diff #63479)	I'd prefer the `#endif` just before `ifunit_ref()`, and then paired `#ifdef/#endif` around `CURVNET_RESTORE()`.

tuexen added inline comments.Oct 19 2019, 7:29 PM

sys/netinet/netdump/netdump_client.c

412–425 ↗

(On Diff #63479)

I expected that you want to do the ifdef dance around CURVNET_RESTORE . The reason I didn't propose it, is that it not done that way (normally) anywhere else in the code...
To avoid that, what about

static int
netdump_configure(struct diocskerneldump_arg *conf, struct thread *td)
{
	struct ifnet *ifp;

	NETDUMP_ASSERT_WLOCKED();

	if (conf->kda_iface[0] != 0) {
		if (td != NULL)
			CURVNET_SET(TD_TO_VNET(td));
		else
			CURVNET_SET(vnet0);
		if (td != NULL && !IS_DEFAULT_VNET(curvnet)) {
			CURVNET_RESTORE();
			return (EINVAL);
		}
		ifp = ifunit_ref(conf->kda_iface);
		CURVNET_RESTORE();
	} else
		ifp = NULL;

	if (nd_ifp != NULL)
...

This avoids all #ifdefs...

cem added inline comments.Oct 19 2019, 7:51 PM

sys/netinet/netdump/netdump_client.c
412–425 ↗	(On Diff #63479)	Sure, that seems fine to me. Thanks!

Integrate result of discussion.

Harbormaster completed remote builds in B27125: Diff 63484.Oct 19 2019, 7:57 PM

tuexen added inline comments.Oct 19 2019, 8:03 PM

sys/netinet/netdump/netdump_client.c
412–425 ↗	(On Diff #63479)	Arrg. It looks nice, but doesn't work in the VIMAGE case, since `CURVNET_SET()` and `CURVNET_RESTORE()` must be in the same block...