mixer: Improve error messages

[fernape]

Summary

Use better diagnostic messages when unit numbers are wrong.

This is an example of the current and proposed behavior:

$ mixer -d pcmAAA
mixer: strtol(AAA): Invalid argument

$mixer -dpcmAAAA
mixer: Can not convert 'AAAA' to a number: Invalid argument

$ mixer -d999999999999999999999999999999999
mixer: strtol(999999999999999999999999999999999): Result too large

$mixer -d99999999999999999999999
mixer: 99999999999999999999999: Result too large

$ mixer -d-2
mixer: cannot set default unit to -2: Invalid argument
pcm3:mixer: <Realtek ALC887 (Front Analog)> on hdaa1 (play/rec) (default)
    vol       = 0.78:0.78     pbk
    pcm       = 0.85:0.85     pbk
    speaker   = 0.00:0.00     rec
    rec       = 0.35:0.35     pbk
    igain     = 0.00:0.00     pbk
    ogain     = 1.00:1.00     pbk
    monitor   = 0.67:0.67     rec src

$mixer -d-2
mixer: Unit number must be a positive number

$ mixer -d99
mixer: cannot set default unit to 99: Invalid argument
pcm3:mixer: <Realtek ALC887 (Front Analog)> on hdaa1 (play/rec) (default)
    vol       = 0.78:0.78     pbk
    pcm       = 0.85:0.85     pbk
    speaker   = 0.00:0.00     rec
    rec       = 0.35:0.35     pbk
    igain     = 0.00:0.00     pbk
    ogain     = 1.00:1.00     pbk
    monitor   = 0.67:0.67     rec src

$mixer -d99
mixer: No such mixer unit: 99

Test Plan

Apply attached patch and run the commands above.

[fernape]

Friendly ping 🔔 !!

[arrowd]

usr.sbin/mixer/mixer.c

76 ↗

(On Diff #177292)

That doesn't look like a clear error message?

[fernape]

usr.sbin/mixer/mixer.c

76 ↗

(On Diff #177292)

Now it does because it removes the strtol text. Or do you think something else? The new behavior is:

mixer -d9999999999999999999999999999999999999999999999999
mixer: 9999999999999999999999999999999999999999999999999: Result too large

[arrowd]

usr.sbin/mixer/mixer.c

76 ↗

(On Diff #177292)

Right, I forgot that err prints a textual errno representation too.

[christos]

To be fair, I don't think these changes are meaningful. mixer(8) will fail for all the cases you are trying to catch anyway, and I think the messages are clear enough already.

Negative dunit:

$ mixer -d -1
mixer: cannot set default unit to -1: Invalid argument

dunit larger than the total number of mixers:

$ mixer -d10
mixer: cannot set default unit to 10: Invalid argument

Too large number:

$ mixer -d9999999999999999999999
mixer: strtol(9999999999999999999999): Result too large

Invalid input:

$ mixer -dfoobar
mixer: strtol(foobar): Invalid argument

The current scheme is uniform, and if a user wants to use the -d option in the first place, they should know that the argument is supposed to be a unit number (or pcm<unit_number>), so the current messages point to what the error is pretty well IMHO.

[fernape]

In D56845#1348954, @christos wrote:

To be fair, I don't think these changes are meaningful. mixer(8) will fail for all the cases you are trying to catch anyway, and I think the messages are clear enough already.

Negative dunit:

$ mixer -d -1
mixer: cannot set default unit to -1: Invalid argument

dunit larger than the total number of mixers:

$ mixer -d10
mixer: cannot set default unit to 10: Invalid argument

Too large number:

$ mixer -d9999999999999999999999
mixer: strtol(9999999999999999999999): Result too large

Invalid input:

$ mixer -dfoobar
mixer: strtol(foobar): Invalid argument

The current scheme is uniform, and if a user wants to use the -d option in the first place, they should know that the argument is supposed to be a unit number (or pcm<unit_number>), so the current messages point to what the error is pretty well IMHO.

I would argue that the user might not know it. "Invalid argument" is barely descriptive IMHO. If mixer knows the problem is that the supplied device number is negative, why not tell the user the exact problem instead of trusting the user "should know" something?

The last two examples (strtol(...) ... Invalid argument) don't make sense from a user point of view. What is strtol()? Is a *user*, not a C developer, suppose to know what that is? mixer *is assuming* the user knows what this C library function does which isn't right I think.
I think explicit messages are better from the user perspective.

[christos]

In D56845#1349251, @fernape wrote:

I would argue that the user might not know it. "Invalid argument" is barely descriptive IMHO. If mixer knows the problem is that the supplied device number is negative, why not tell the user the exact problem instead of trusting the user "should know" something?

The reason I'm pushing back against this is because, apart from the fact that the -d option is already documented in the man page, the logic of the option is that you supply the name of the device (e.g., pcm0) or just its unit number. The error messages you introduce are accurate in the sense that they are more fine-grained in an input sense, but the point is that the user is trying to reference a device that does not exist, and these messages in my opinion make the issue more about the fact that the numbers are negative/out of range/etc, and not about the fact that the device simply does not exist. That's why I think "Invalid argument" for example is enough, because if someone deliberately tries to use the -d option, they should know what arguments it expects also, they wouldn't use an option randomly.

The last two examples (strtol(...) ... Invalid argument) don't make sense from a user point of view. What is strtol()? Is a *user*, not a C developer, suppose to know what that is? mixer *is assuming* the user knows what this C library function does which isn't right I think.
I think explicit messages are better from the user perspective.

I will agree that the "strtol" string is not needed here. I do not object to removing "strtol" and just leaving it as:

err(1, "%s", optarg);

[kevans]

In D56845#1352108, @christos wrote:

In D56845#1349251, @fernape wrote:

I would argue that the user might not know it. "Invalid argument" is barely descriptive IMHO. If mixer knows the problem is that the supplied device number is negative, why not tell the user the exact problem instead of trusting the user "should know" something?

The reason I'm pushing back against this is because, apart from the fact that the -d option is already documented in the man page, the logic of the option is that you supply the name of the device (e.g., pcm0) or just its unit number. The error messages you introduce are accurate in the sense that they are more fine-grained in an input sense, but the point is that the user is trying to reference a device that does not exist, and these messages in my opinion make the issue more about the fact that the numbers are negative/out of range/etc, and not about the fact that the device simply does not exist. That's why I think "Invalid argument" for example is enough, because if someone deliberately tries to use the -d option, they should know what arguments it expects also, they wouldn't use an option randomly.

I'd argue that the ERANGE check should set dunit = INT_MAX or so, and the dunit < 0 check should move into their new branch in set_dunit (nit > mixer_get_nmixers())) because "No such mixer unit: mixer-1" or whatnot would probably be a net improvement over "mixer: cannot set default unit to -1: Invalid argument" (although, a marginal improvement, to be sure).

[fernape]

Remove error msg about unit number being negative

[fernape]

In D56845#1352108, @christos wrote:

In D56845#1349251, @fernape wrote:

I would argue that the user might not know it. "Invalid argument" is barely descriptive IMHO. If mixer knows the problem is that the supplied device number is negative, why not tell the user the exact problem instead of trusting the user "should know" something?

The reason I'm pushing back against this is because, apart from the fact that the -d option is already documented in the man page, the logic of the option is that you supply the name of the device (e.g., pcm0) or just its unit number. The error messages you introduce are accurate in the sense that they are more fine-grained in an input sense, but the point is that the user is trying to reference a device that does not exist, and these messages in my opinion make the issue more about the fact that the numbers are negative/out of range/etc, and not about the fact that the device simply does not exist. That's why I think "Invalid argument" for example is enough, because if someone deliberately tries to use the -d option, they should know what arguments it expects also, they wouldn't use an option randomly.

Fine by me. Thanks for taking it into consideration.

The last two examples (strtol(...) ... Invalid argument) don't make sense from a user point of view. What is strtol()? Is a *user*, not a C developer, suppose to know what that is? mixer *is assuming* the user knows what this C library function does which isn't right I think.
I think explicit messages are better from the user perspective.

I will agree that the "strtol" string is not needed here. I do not object to removing "strtol" and just leaving it as:

err(1, "%s", optarg);

Thanks, that would be nice.

[christos]

usr.sbin/mixer/mixer.c

74 ↗

(On Diff #184364)

This could be omitted IMHO.

[fernape]

Improve message for wrong pcm device numbers

[fernape]

usr.sbin/mixer/mixer.c

74 ↗

(On Diff #184364)

Actually I realized that both in my change and in the original code, this might happen:

$ ./mixer -dpcm
mixer: : Invalid argument

Now the behavior would be:

$ ./mixer -dpcm
mixer: pcm: Invalid argument

$ ./mixer -daaa
mixer: aaa: Invalid argument

[christos]

usr.sbin/mixer/mixer.c

74 ↗

(On Diff #184691)

Why are you hardcoding "pcm" here?

336 ↗

(On Diff #184691)

This could go at the top so that we don't waste time fetch n if dunit is out of bounds.

[fernape]

Fast exit on unknown device

[fernape]

usr.sbin/mixer/mixer.c

74 ↗

(On Diff #184691)

If optarg is not "pcm", then we don't advance optarg so strlen(optarg) is not zero, and we print optarg whatever that is. The only case in which we can get an empty optarg is if we have advanced optarg for the pcm case.

$ ./mixer -dfoo
mixer: foo: Invalid argument
$ ./mixer -dfoo8
mixer: foo8: Invalid argument
$ ./mixer -dpcm
mixer: pcm: Invalid argument
$ ./mixer -dpcm7
mixer: No such mixer unit: 7
$ ./mixer -dpcm2
default_unit: 5 -> 2

[fernape]

Thank you very much for the review!

[christos]

No problem! Make sure you MFC this to stable/15 at some point.

[ngie]

You linked the wrong commit to the wrong revision. Could you please fix this?

[ngie]

This comment was removed by ngie.