can errno ever be EEXIST here?

Sure. This is what a normal user will get if cuse(3) is not loaded:

Failed to load cuse kernel module: Operation not permitted

Because regular users can't load kernel modules.

EEXIST will be returned if the user can try to load the kernel module, but it is already loaded.

Why did you move this further up? parse_options() can call dup_profile() which creates a cuse device, so if the module is not loaded already before virtual_oss runs, the call should fail. Haven't tested this though, but I don't understand why this block is moved.

This is what my inline comment above refers to. You've moved parse_options() above kldload(cuse.ko) and cuse_init(), so the calls to cuse_dev_create() from dup_profile() will fail.

I tried several options like:

virtual_oss  -f /dev/dsp3  -c 1  -r 48000  -b 16  -s 4800  -m 0,0  -d virtual_mic
virtual_oss     -b 16     -c 1     -r 48000     -s 4800     -f /dev/dsp3     -m 0,0     -e 1,0     -d virtual_mic

and others, so that one might have slipped.

I moved it up, because previous to this change, doing virtual_oss -h for a regular user, if cuse is not loaded results in an error which is no good at all.

We can try several options here. One is that virtual_oss might not try to load the module by itself. Another one would be to decouple at least the -h option so it can be shown to the user should the module is not loaded. For dup_provile() and maybe others, check first if the module is loaded, etc.

I don't understand what was wrong with the current behavior. If cuse wasn't even loaded, we'd print an error message. There is no reason to print usage() here because this isn't a virtual_oss usage error, it's that virtual_oss needs cuse.ko loaded in order to work, and that message was printed already.

The reason you didn't hit the error I'm mentioning is because you run virtual_oss as a plain virtual_oss command, but no one runs it like this because that's essentially a no-op. With this patch applied normal virtual_oss invocations fail every single time because cuse_init() is called after parse_options().

I'm reverting the patch.

I don't understand what was wrong with the current behavior. If cuse wasn't even loaded, we'd print an error message. There is no reason to print usage() here because this isn't a virtual_oss usage error, it's that virtual_oss needs cuse.ko loaded in order to work, and that message was printed already.

The reason you didn't hit the error I'm mentioning is because you run virtual_oss as a plain virtual_oss command, but no one runs it like this because that's essentially a no-op. With this patch applied normal virtual_oss invocations fail every single time because cuse_init() is called after parse_options().

Well, I'd argue it is pretty common behavior to execute a command either without options and get the help by default or by using -h and none works. But nevertheless, this is the behavior in 15.1 which is pretty tough on users:

fernape~$ kldstat | grep cuse
fernape~$ virtual_oss -h
virtual_oss: Failed to load cuse kernel module: Operation not permitted
fernape~$ sudo kldload cuse
Password:
fernape~$ virtual_oss -h
virtual_oss: Failed to load cuse kernel module: Operation not permitted
fernape~$ kldstat | grep cuse
22    1 0xffffffff832d1000     6710 cuse.ko
fernape~$ uname -a
FreeBSD beastie 15.1-RELEASE-p3 FreeBSD 15.1-RELEASE-p3 releng/15.1-n283611-88e7371d9dc2 GENERIC amd64

I'm reverting the patch.

Thanks for reverting.

We do print usage messages in general, but at this point in the execution the program is still being set up, so a usage() could even be misleading because it's not a usage error.

Also I'm not sure why you get an EPERM from kldload() even though you have the module loaded. Normally it should return EEXIST (i.e., already loaded), and you should fail at cuse_init() instead:

$ kldstat | grep cuse
 5    1 0xffffffff84148000     6710 cuse.ko
$ virtual_oss
virtual_oss: Could not connect to cuse module

In any case, virtual_oss is meant to be run as root because it needs to create device nodes. I'm not sure it's a good idea to essentially getopt() twice, once at the beginning, once in parse_options(), just so that -h can work on a pretty much wrong invocation (non-root).

We do print usage messages in general, but at this point in the execution the program is still being set up, so a usage() could even be misleading because it's not a usage error.

Also I'm not sure why you get an EPERM from kldload() even though you have the module loaded. Normally it should return EEXIST (i.e., already loaded), and you should fail at cuse_init() instead:

Because as I said in the summary, kldload checks permissions first (as it should) and if the user can't load modules, just returns with -EPERM. It is only if the user is privileged enough to load modules, that later on, the presence of the module in memory is checked and if so -EEXIST is returned. So virtual_oss is not handling return errors from kldload properly.

$ kldstat | grep cuse
 5    1 0xffffffff84148000     6710 cuse.ko
$ virtual_oss
virtual_oss: Could not connect to cuse module

In any case, virtual_oss is meant to be run as root because it needs to create device nodes. I'm not sure it's a good idea to essentially getopt() twice, once at the beginning, once in parse_options(), just so that -h can work on a pretty much wrong invocation (non-root).

Why is virtual_oss meant to be run as root? Why if the devices have the proper permissions for the user because they were configured that way? If virtual_oss is meat to be run as root, it should do that check in the very first line in main(), which it doesn't.

Please, if you can, have a look at https://reviews.freebsd.org/D59844

At least that change allows a regular user to access the help via -h if the module has already been loaded.

Why is virtual_oss meant to be run as root? Why if the devices have the proper permissions for the user because they were configured that way? If virtual_oss is meat to be run as root, it should do that check in the very first line in main(), which it doesn't.

Since a6aa95eba6ee2dd199d6cb8063dbaccd8a6f7512, all audio device nodes are created with GID_AUDIO and users who want to access them have to be part of the audio group.

Apart from that, virtual_oss calls, for example, kldload() which needs root access in order to run in the first place, as per kldload.2. This is partly the reason why I'm saying the patch is rather unnecessary.

Why is virtual_oss meant to be run as root? Why if the devices have the proper permissions for the user because they were configured that way? If virtual_oss is meat to be run as root, it should do that check in the very first line in main(), which it doesn't.

Since a6aa95eba6ee2dd199d6cb8063dbaccd8a6f7512, all audio device nodes are created with GID_AUDIO and users who want to access them have to be part of the audio group.

Regardless of the permissions of the devices?

Apart from that, virtual_oss calls, for example, kldload() which needs root access in order to run in the first place, as per kldload.2. This is partly the reason why I'm saying the patch is rather unnecessary.

Not for -h, right? Let me show an example of how this is done in other places:

$ pkg -v
2.8.4
$ pkg install nvidia-driver
pkg: Insufficient privileges to install packages

The operations that can be run without root, are run.
The operations that need root, show a very descriptive message about what's going on.

In any case, can you answer these two questions?

• Why virtual_oss.8 does not mention anything about the necessity of using root to run it?
• Why doesn't virtual_oss(8) check for uid 0 at startup?

The control flow of virtual_oss at its current form is indeed a bit strange in some places, and I want to improve it as well, but parse_options() would have to be refactored too, and I've been deferring this for now, since I didn't deem it critical.

• virtual_oss.8 does not mention the need to be run as root because a simple invocation will make it clear immediately. pkg-install.8 that you mentioned also does not mention this.
• It doesn't check for uid 0 at startup because it is not that we want virtual_oss to be run as root, it's just that certain functions like kldload(2) require root, so they can do the checks.

Also audio devices can be accessed from any uid, as long as they are part of the audio group.

The control flow of virtual_oss at its current form is indeed a bit strange in some places, and I want to improve it as well, but parse_options() would have to be refactored too, and I've been deferring this for now, since I didn't deem it critical.

Don't get me wrong, I do appreciate the work done here. I just found rough edges because I happened to be in the need of virtual_oss to create a virtual microphone.

• virtual_oss.8 does not mention the need to be run as root because a simple invocation will make it clear immediately. pkg-install.8 that you mentioned also does not mention this.

To be fair in the comparison, pkg install message is much more clear than the one from virtual_oss by a good margin.

• It doesn't check for uid 0 at startup because it is not that we want virtual_oss to be run as root, it's just that certain functions like kldload(2) require root, so they can do the checks.

I understood that virtual_oss was meant to be run as root. I think it'd be nice if virtual_oss were free to run for users and just showed an informative message when an operation requires privileges.

Also audio devices can be accessed from any uid, as long as they are part of the audio group.

I understood that virtual_oss was meant to be run as root. I think it'd be nice if virtual_oss were free to run for users and just showed an informative message when an operation requires privileges.

I agree with this. The only big barrier here is the cuse.ko kldload, which is required in order for virtual_oss to work. The only way to avoid it is to expect cuse.ko to be pre-loaded by the user and fail otherwise, but I'm not sure if that'd improve user experience.