Re: [libvirt PATCH] drivers: Group global feature together

On a Thursday in 2022, Andrea Bolognani wrote: > On Wed, Feb 16, 2022 at 09:26:21PM +0100, Ján Tomko wrote: > > > switch ((virDrvFeature) feature) { > > > + case VIR_DRV_FEATURE_REMOTE: > > > + case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE: > > > + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK: > > > + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK: > > > case VIR_DRV_FEATURE_TYPED_PARAM_STRING: > > > case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: > > > - return 1; > > > + case VIR_DRV_FEATURE_FD_PASSING: > > > + /* Should have already been handled by virDriverFeatureIsGlobal() */ > > > + return -1; > > > > Here you return an error without reporting an error. > > > > Would virReportEnumRangeError be reasonable to use here? > > Mh. While the error message ("Unexpected enum value N for virFoo") is > not too specific about this, virReportEnumRangeError() is usually > called when the provided value cannot be translated back to one of > the enum values; in this case it *can* be converted, it's just not > supposed to be encountered in the specific situation. Given the > subtly different semantics, I'm leaning towards not using that > helper. > > Would something like > > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("Global feature %d should have already been handled"), > feature); > > work for you? I'd drop the comment of course. Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt;