It could. I mostly (out of habit) wanted to normalize the index to 0-based as soon as possible and pass around a 0-based index, even if there is a little duplication. Mixing up 0- and 1-based indices happens and the easiest way to avoid that is to normalize as soon as possible.

In this case, I think it would be ok to push the 1-based index down into insert_dumper.

(The motivation for 1-based indexing at all is to retain (some) ioctl ABI compatibility with 12.0, in that kda_enable/kda_index of 0 still means "disable," and kda_enable/kda_index of 1 still means "configure this dump device on.")

Sure, that seems like a good idea to me. Will fix.

There's a lot of other sysctl handlers which happily wire user memory without such a check, and in general unprivileged users are permitted to wire a small amount (more than a page) of physical memory anyway. Rather than having ad-hoc policies implemented in sysctl handlers, I'd prefer to handle the issue globally in vslock() (which currently gives you a very loose seatbelt).

You can replace the flag variable with if (di == TAILQ_FIRST(&dumper_configs)).

I know encryption and compression can't be configured simultaneously right now, but that could easily change in the future. Is there any other reason to use an else if here rather than plain if?

Most of these parameters are extracted from a diocskerneldump_arg, why not just pass that structure directly like with dumper_remove()?

On the one hand, sure. On the other hand, to exceed a 4k page, even with ~255 devices (theoretical max based on uint8_t kda_index), would take just over 16 bytes per device name on average (which is much longer than typical for network devices or even disk partitions). It's just a relatively huge amount of memory relative to what is needed and also the minimum unit we can wire, so I figured it was pretty reasonable.

I'm happy to remove it, though. Will plan to do that.

Ok

This branch isn't related to our current compression+encryption limitation. It's just asserting that a non-NONE kda_compression mode doesn't match kdcomp == NULL. Plain if would require restating kdcomp == NULL && which we inherit via else if.

I didn't set out to rototill the interface that much. I don't have any good argument against doing so, though. I agree that passing a struct with arguments would be more sensible than 10s of parameters which can be easily misordered, and diocskerneldump_arg is probably the logical choice. I'll make that change.

More correct would be “unprivileged.” We shouldn’t crash if root manipulates a kernel data structure concurrently with access either, but it’s slightly better. I will probably remove the adjective and just mention concurrent manipulation and the sysctl.

Does “These are...” make the comment easier to understand? I doubt it, though it improves style(9) conformance. I inherited this one, just added “(s)”. Will fix for style sake.

I don’t have a strong preference for either. I’d guess there are more people confused about this than harmed by memcpy, so I’m happy to change it.

Yes - this allows things like “dumpon off” to continue to disable everything without iterating all dump configurations explicitly. Are you thinking the sys/disk.h comment could specify that more clearly? For that matter, it’s probably more appropriate in the .c implementation anyway.

I just meant to capitalize and add a period since the comment is on its own line now.

It might be worth noting that wildcard matches are permitted.

Can't — it must be in-scope after leaving that case statement's embedded block due to the fallthrough.

Hm, it'll be another ABI break too. That's a bit unfortunate; I'd like to avoid a second round of compatibility ABI for older versions of head. Maybe we should just make that change now (without adding ipv6 support to dumpon/netdump, just adding union types to the ABI interface)?

It'd require minor fiddling in NETDUMPGCONF_FREEBSD12, but otherwise should be straightforward, I think.

(I don't see any good named union type for in_addr+in6_addr — outside of ofed's cma_ip_addr — but it's a widely used construct in the tree — nfs, tcp_lro, tcp_fastopen, ip_fw, pf, if_llatbl all seem to have some kind of definition.) There's in_dependaddr but I'm not sure in_addr_4in6 is the most appropriate spelling of in_addr given the way we would use it. I'm leaning towards defining a new type.

Ah, I thought it was permitted to refer to variables outside of their lexical scope so long as you're still in the same function.

We don't provide compatibility guarantees in head, so we can just break the interface. (Hence "before 13.0.")

I think we can (ab)use struct sockaddr?

I don't see what's wrong with sockaddr_storage. It's larger than is needed but that doesn't matter for a rarely executed ioctl, and it's standard to use that in code that is address family-agnostic. And sys/_sockaddr_storage.h will introduce less pollution in sys/disk.h.

Nothing wrong with sockaddr_storage, really. Mostly it just makes the interface more general in a way that isn't helpful (requires more boilerplate to check for EINVAL input). We can use it instead, if you would prefer.

I think I'd still prefer it to a hand-rolled structure. Is the extra boilerplate just that we have to check the AF of each field instead of one?

Yeah. And whatever ambiguity surrounds other fields that matter for sockets but not hosts, eg flowid, port, etc.

Also that sockaddr_storage is an array type rather than a union and all real access requires ugly casting; and we don't save anything in header includes because we need net/if.h which means we pull in the in/in6 addr types already.

I've rototilled sockaddr_storage into just dumpon alone (to give it a shot) and it is really really ugly. And there would be a large number of additional churn involving sockaddr_in casts in netdump_client.c to finish the job. I don't think sockaddr_storage is a suitable type for this use case (and it's only used with BSD sockets for legacy API reasons).

So I don't plan to continue converting to sockaddr_storage — I'll just leave it with the address union instead.