Re: [PATCH 09/14] error: Use error_setg_file_open() for simplicity and consistency

[Markus Armbruster <armbru@xxxxxxxxxx>]

"Dr. David Alan Gilbert" <dave@xxxxxxxxxxx> writes:

> * Markus Armbruster (armbru@xxxxxxxxxx) wrote:
>> "Dr. David Alan Gilbert" <dave@xxxxxxxxxxx> writes:
>> 
>> > * Markus Armbruster (armbru@xxxxxxxxxx) wrote:
>> >> Replace
>> >> 
>> >>     error_setg_errno(errp, errno, MSG, FNAME);
>> >> 
>> >> by
>> >> 
>> >>     error_setg_file_open(errp, errno, FNAME);
>> >> 
>> >> where MSG is "Could not open '%s'" or similar.
>> >> 
>> >> Also replace equivalent uses of error_setg().
>> >> 
>> >> A few messages lose prefixes ("net dump: ", "SEV: ", __func__ ": ").
>> >> We could put them back with error_prepend().  Not worth the bother.
>> >
>> > Yeh, I guess you could just do it with another macro using
>> > the same internal function just with string concatenation.
>> 
>> I'm no fan of such prefixes.  A sign of developers not caring enough to
>> craft a good error message for *users*.  *Especially* in the case of
>> __func__.
>> 
>> The error messages changes in question are:
>> 
>>     net dump: can't open DUMP-FILE: REASON
>>     Could not open 'DUMP-FILE': REASON
>> 
>>     SEV: Failed to open SEV-DEVICE: REASON
>>     Could not open 'SEV-DEVICE': REASON
>> 
>>     sev_common_kvm_init: Failed to open SEV_DEVICE 'REASON'
>>     Could not open 'SEV-DEVICE': REASON
>> 
>> I think these are all improvements, and the loss of the prefix is fine.
>
> Yeh, although I find the error messages aren't just for users;
> they're often for the first dev to see it to guess which other
> dev to pass the problem to, so a hint about where it's coming
> from can be useful.

I agree!  But I think an error message must be make sense to users
*first* and help developers second, and once they make sense to users,
they're often good enough for developers.

The common failures I see happen when developers remain caught in the
developer's perspective, and write something that makes sense to *them*.
Strawman form:

    prefix: failed op[: reason]

where "prefix" is a subsystem tag, or even __func__, and "reason" is
strerror() or similar.

To users, this tends to read as

    gobbledygook: techbabble[: reason]

When we care to replace "failed op" (developer's perspective) by
something that actually makes sense to users, "prefix" often becomes
redundant.

The error messages shown above aren't bad to begin with.  "failed to
open FILE", where FILE is something the user specified, should make
sense to the user.  It should also be good enough for developers even
without a prefix: connecting trouble with the DUMP-FILE to dump /
trouble with the SEV-DEVICE to SEV should be straightforward.

[...]