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
[libvirt] [PATCH v2 02/42] util: handle missing switch enum cases
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; } return 0; @@ -986,13 +989,21 @@ int virConfGetValueStringList(virConfPtr conf, } ATTRIBUTE_FALLTHROUGH; -default: +case VIR_CONF_LLONG: +case VIR_CONF_ULLONG: +case VIR_CONF_NONE: virReportError(VIR_ERR_INTERNAL_ERROR, compatString ? _("%s: expected a string or string list for '%s' parameter") : _("%s: expected a string list for '%s' parameter"), conf->filename, setting); return -1; + +case VIR_CONF_LAST: +default: +virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected value type %d"), cval->type); +return -1; } return 1; diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index ff1bb83073..e7da482640 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -827,9 +827,12 @@ virFirewallApplyRule(virFirewallPtr firewall, if (virFirewallApplyRuleFirewallD(rule, ignoreErrors, &output) < 0) return -1; break; + +case VIR_FIREWALL_BACKEND_AUTOMATIC: +case VIR_FIREWALL_BACKEND_LAST: default: -virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unexpected firewall engine backend")); +virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected firewall engine backend %d"), currentBackend); return -1; } diff --git a/src/util/virlog.c b/src/util/virlog.c index 4f66cc5e5c..ecbee71cfb 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1206,10 +1206,18 @@ virLogGetOutputs(void) virLogDestinationTypeToString(dest), virLogOutputs[i]->name); break; -default: +case VIR_LOG_TO_STDERR: +case VIR_LOG_TO_JOURNALD: virBufferAsprintf(&outputbuf, "%d:%s", virLogOutputs[i]->priority, virLogDestinationTypeToString(dest)); +break; +case VIR_LOG_TO_OUTPUT_LAST: +default: +virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected log output type %d"), dest); +virLogUnlock(); +return NULL; } } virLogUnlock(); diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index db495a7549..4c0a64e439 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -1071,6 +1071,9 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname, case VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE: op = PORT_REQUEST_PREASSOCIATE; break; +case VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE_RR: +op = PORT_REQUEST_PREASSOCIATE_RR; +break; case VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE: op = PORT_REQUEST_ASSOCIATE; break; @@ -1191,10 +1194,16 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname, false); break; -default: +case VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE: virReportError(VIR_ERR_INTERNAL_ERROR, _("operation type %d not supported"), virtPortOp); rc = -1; +break; +default: +virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected operation type %d"), virtPortOp); +rc = -1; +break; } cleanup: -- 2.14.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list