[libvirt] [PATCH v2 02/42] util: handle missing switch enum cases

[John Ferlan]

On 02/20/2018 06:25 AM, Daniel P. Berrangé wrote:
> On Tue, Feb 20, 2018 at 12:19:15PM +0100, Andrea Bolognani wrote:
>> On Tue, 2018-02-20 at 09:54 +0000, Daniel P. Berrangé wrote:
>>>>> +        case VIR_CONF_LAST:
>>>>>         default:
>>>>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>>>>> +                           _("Unexpected conf value type %d"), val->type);
>>>>>             return -1;
>>>>
>>>> All these errors are presumably dead code that we only keep around in
>>>> case we broke something in other parts of the code.
>>>>
>>>> Do we need specific user-friendly translated errors? Since we log the
>>>> function name as well, something like: "unhandled enum value %d" would
>>>> do.
>>>
>>> The function name only gets into the logs - not the error reporting,
>>> so if someone does get an error raised, I don't want it to be a totally
>>> generic message that could come from literally anywhere in the codebase.
>>
>> Yesterday I argued in a different thread that it would be better
>> to include the enum name in the error message, since that's useful
>> information for developers whereas users 1) should never see this
>> kind of error to begin with and 2) when they do, their only course
>> of action is reporting the issue anyway.
> 
> How about we standard it via a special API
> 
>    virReportErrorEnumRange(virDomainControllerModelUSB, val->type);
> 
> and map this through to a VIR_ERR_ENUM_RANGE error code, with a fixed
> string format.
> 
>    "Value '%d' out of range for enum %s"
> 
> Regards,
> Daniel
> 

This works, but is it really necessary? Essentially for developers only?
It's essentially a can't get there and val->type is either _LAST or some
value outside the range of all possible values.

The error messages I've seen for this throughout this series are
generally the same "Unexpected XXX %d" as opposed to perhaps
"Unsupported XXX ..." when values are within the range, but essentially
wrong. The errors also used the INTERNAL_ERR code rather than
UNSUPPORTED or XML_ERR, so I would believe it's really easy for even a
semi-conscious developer to figure out.

Whether the "Unexpected..." is provided has been on a function by
function basis based on the knowledge of the code path and that for
certain paths (such as Free functions) it makes no sense to print anything.

Personally I find the consistency of using "Unexpected" better than
value out of range. If the using some sort of an inline helper to ensure
to record the line number of the calling function is more desired, then
let's also create a new error code to really single it out.

John

(hopefully this all makes sense - only 1/2 cup of coffee in ;-))