Re: [libvirt] [PATCH v2 02/42] util: handle missing switch enum cases
On Tue, Feb 20, 2018 at 01:05:23PM +0100, Andrea Bolognani wrote: > On Tue, 2018-02-20 at 11:25 +, Daniel P. Berrangé wrote: > > > 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" > > Sounds like a good idea! We could even add something like > > This is a bug in libvirt, please report it. > > or similar to make it clear that the user is not at fault. I don't think we should go down that road - most errors are not the user's fault - they the fault of some component somewhere in the stack. > > Not sure about using a separate error code rather than the existing > INTERNAL_ERROR, though: it seems like it would not really buy us > anything. > > -- > Andrea Bolognani / Red Hat / Virtualization Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/42] util: handle missing switch enum cases
On Tue, 2018-02-20 at 11:25 +, Daniel P. Berrangé wrote: > > 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" Sounds like a good idea! We could even add something like This is a bug in libvirt, please report it. or similar to make it clear that the user is not at fault. Not sure about using a separate error code rather than the existing INTERNAL_ERROR, though: it seems like it would not really buy us anything. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/42] util: handle missing switch enum cases
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 +, 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 ;-))
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/42] util: handle missing switch enum cases
On Tue, Feb 20, 2018 at 12:19:15PM +0100, Andrea Bolognani wrote:
> On Tue, 2018-02-20 at 09:54 +, 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
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/42] util: handle missing switch enum cases
On Tue, 2018-02-20 at 09:54 +, 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.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/42] util: handle missing switch enum cases
On Tue, Feb 20, 2018 at 10:49:29AM +0100, Ján Tomko wrote:
> On Thu, Feb 15, 2018 at 04:43:07PM +, Daniel P. Berrangé wrote:
> > Ensure all enum cases are listed in switch statements.
> >
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > src/util/virconf.c | 13 -
> > src/util/virfirewall.c | 7 +--
> > src/util/virlog.c| 10 +-
> > src/util/virnetdevvportprofile.c | 11 ++-
> > 4 files changed, 36 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/util/virconf.c b/src/util/virconf.c
> > index a82a509ca3..af806dd735 100644
> > --- a/src/util/virconf.c
> > +++ b/src/util/virconf.c
> > @@ -296,7 +296,10 @@ virConfSaveValue(virBufferPtr buf, virConfValuePtr val)
> > virBufferAddLit(buf, " ]");
> > break;
> > }
> > +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.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/42] util: handle missing switch enum cases
On Thu, Feb 15, 2018 at 04:43:07PM +, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements.
Signed-off-by: Daniel P. Berrangé
---
src/util/virconf.c | 13 -
src/util/virfirewall.c | 7 +--
src/util/virlog.c| 10 +-
src/util/virnetdevvportprofile.c | 11 ++-
4 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/src/util/virconf.c b/src/util/virconf.c
index a82a509ca3..af806dd735 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -296,7 +296,10 @@ virConfSaveValue(virBufferPtr buf, virConfValuePtr val)
virBufferAddLit(buf, " ]");
break;
}
+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.
Jan
}
return 0;
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/42] util: handle missing switch enum cases
On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote: > Ensure all enum cases are listed in switch statements. and in some cases force an error if something unexpected is found. [not necessary to add, especially since it's repeatable throughout the series so far, but figured I'd note it at least B-)] > > Signed-off-by: Daniel P. Berrangé > --- > src/util/virconf.c | 13 - > src/util/virfirewall.c | 7 +-- > src/util/virlog.c| 10 +- > src/util/virnetdevvportprofile.c | 11 ++- > 4 files changed, 36 insertions(+), 5 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
