On Thu, Apr 07, 2022 at 02:24:05PM +0200, Ján Tomko wrote:
> 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@xxxxxxxxxx>
Thanks! Pushed after squashing in the virReportError() calls and
tweaking the error message slightly.
--
Andrea Bolognani / Red Hat / Virtualization
