Re: [libvirt] [PATCH 2/3] datatypes: avoid redundant __FUNCTION__
On Wed, May 12, 2010 at 10:31:50AM +0200, Matthias Bolte wrote:
2010/5/12 Eric Blake <eblake(a)redhat.com>:
<snip>
As Chris pointed out a while ago:
https://www.redhat.com/archives/libvir-list/2010-April/msg01241.html
There are some error codes that have string representations attached
which imply a certain usage style. VIR_ERR_INVALID_ARG is one of
those. See virErrorMsg:
case VIR_ERR_INVALID_ARG:
if (info == NULL)
errmsg = _("invalid argument in");
else
errmsg = _("invalid argument in %s");
break;
This implies that you pass the function name as additional
information. But I must admit that I never used VIR_ERR_INVALID_ARG as
it is implied, because I wasn't aware of that until recently.
The VIR_ERR_XML_ERROR is an even worse example:
case VIR_ERR_XML_ERROR:
if (info == NULL)
errmsg = _("XML description not well formed or invalid");
else
errmsg = _("XML description for %s is not well formed
or invalid");
break;
Unrelated to your patch I suggest that we unify the string
representations for error codes to a common style:
case VIR_ERR_INVALID_ARG:
if (info == NULL)
errmsg = _("invalid argument");
else
errmsg = _("invalid argument: %s");
break;
case VIR_ERR_XML_ERROR:
if (info == NULL)
errmsg = _("XML description not well formed or invalid");
else
errmsg = _("XML description not well formed or invalid: %s");
break;
And adapt the callers.
+1
A common style for error messages is the right way to go, and I like
the style Matthias proposes of
"general error message: %s", "specific error"
IMO, most of the time outputting a function name in an error message
only obscures the message, and in the cases in which it's actually
useful information, the writer of the message can always include it
manually. The same goes for pointer addresses.
I think this rework is a real usability enhancement. If we have
consensus that we should do it, I'll submit patches for the storage
and node device code.
Dave