I have no idea why Arcanist decided to include all of those extra revisions when I updated the diff. I did not rebase. Only the most recent three revisions are intended to be part of the review. I'll abandon and reopen.

• Abandon the custom kernel module. Rely on existing syscalls instead.
• move the test into the kern directory

The fusefs part LGTM. I didn't read the rest.

In D51081#1165925, @olivier wrote:

In D51081#1165915, @asomers wrote:

This is ok. It should normally be unnecessary, though, because ctladm should always be installed. Are you using pkgbase or something? I guess it might be optional in a pkgbase world.

Yes, indeed, it should now always be installed. However, we have a custom world (WITHOUT_ISCSI) and kernel (no device ctl), and since ctladm is no longer gated behind WITHOUT_ISCSI (commit 08419bf19469), this is the cleanest way I’ve found to fix this specific use case.

This is ok. It should normally be unnecessary, though, because ctladm should always be installed. Are you using pkgbase or something? I guess it might be optional in a pkgbase world.

In D50826#1161127, @arrowd wrote:

LGTM, but you're still ORing POLLHUP into revents, not setting it. As I mentioned in comment, it seems that the FreeBSD way to report EOF is to return POLLHUP only.

In D50831#1161443, @kib wrote:

We did not finished with stabilizing the kernel interface yet, and not much started looking at the proper userspace facilities. It might end up built-in into err()/warn() so uexterr_gettext() would be left as some internal function. I simply did not considered that we are there yet.

For the exterrctl(2), the man page would be eventually added, but with the warning similar to that for thr*(2) and sigfastblock(2) that the interface is for C runtime and should not be used by the app code.

In D50831#1161438, @arrowd wrote:

What man page? The information on how to read exterrors is located in kdumps manpage.

This change is ok with me. But I would still like to see a man page and some usage examples from userland.

This is an interesting new feature. But how do we use it? There aren't any examples yet, or any man page. We could easily add test cases for the mount and kqueue errors to the fusefs test suite.

• fixup: output POLLHUP regardless of the contents of events

In D50799#1159803, @arrowd wrote:

LGTM, but my review doesn't worth much.

• Respond to gleb's comments

A redeclaration of an entity without a linkage specification inherits the language linkage of the entity and its type (if exists).

A few functions, like conf_set_debug, will now be extern "C" in the header file, but not in their implementation. It might work, but I don't think it's right. Also, conf.c seems to have two purposes. It defines C functions used by parse.y, and it also defines C functions used by ctld.cc. Maybe we should leave conf.c as a C file, but move some its ctld functions into ctld.cc itself? OTOH, if you're going for "minimal set of changes necessary to build as C++, it would be easier just to move those prototypes out of the "extern C" section.

This LGTM. C++20 would be nicer, because it would simplify one thing in the fusefs tests, but that would require a whole new tinderbox run.

LGTM. Except that the commit message contains a typo: "uprivileged_user" => "unprivileged_user"

What maintenance burden, @kp? I think it's a worthwhile change. I've run into similar problems myself. I find it easy to have a system where something pulls in, say, lang/python311 but not lang/python3.

Ahh, sorry for the noise.

The old code certainly looks broken. I guess nobody was using it?

It's frustrating that file parsing is so difficult in C.

Yikes. I guess nobody was using those.

Did you check for any similar resource leaks in uclparse.c ?

Thanks for doing all this cleanup.

In D48798#1115132, @jhb wrote:

FYI, one hiccup with this plan is that yacc only generates C code. I am working on creating a dedicated C API to sit between parse.y and ctld.c so that the rest of ctld can be C++. Blech.

In D48798#1114872, @jhb wrote:

I'm somewhat torn on doing a big change while the nvme stuff is in flight, but I do think maybe we should just bite the bullet. git can handle renames pretty sanely for rebasing I think.

So what I think I would like to do is go ahead and convert ctld(8) to C++. The first commit would be to just rename all the .c files to .cc and change the Makefile, but make no other functional changes. We might need to a prep commit to add BEGIN_DECLS and END_DECLS to libiscsiutil.h. After that lands, you could rebase this and use C++ enum classes for these. I might also use classes instead of my protocol_ops structure in the nvme vs iscsi changes. It might be nice to use classes for several of the data structures in fact including auth_groups, portal_groups, etc. Also while updating certain things it might nice to use STL containers in place of queue macros, but that can be done incrementally.

Thank you!

The only improvement I could suggest would be to factor out the guts of both functions into a third, private function. That would save callers of nvmf_nqn_valid_strict from calling strlen twice. But it's probably premature optimization.

I think this is ok. Losing the ability to apply configuration once before daemonizing is unfortunate but understandable. If it's a problem, we can fix it later by using rfork. I also see that you're only passing a single-element eventlist to kevent, which isn't the most efficient. But ctld isn't supposed to get a high rate of connections, so that's fine.

• fixup: add eventfd too

In D48650#1109739, @markj wrote:

This change addresses the failing test, but there's a new failure in sys/netinet/fibs_test:same_ip_multiple_ifaces_fib0. The test creates two interfaces, then assigns the same IP address to each, with different masks. With this change, the second address assignment fails with EEXIST.

I'm not sure if that's actually a valid thing to do in practice; see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=189089 . Maybe @asomers remembers?