Re: [libvirt] [PATCH 3/4] util: Add 'label' field to VIR_ENUM_IMPL

On Thu, Apr 11, 2019 at 12:27:56PM -0400, Cole Robinson wrote: > On 4/11/19 6:57 AM, Daniel P. Berrangé wrote: >> On Mon, Apr 08, 2019 at 11:48:18AM -0400, Cole Robinson wrote: >>> This allows us to raise error messages from virEnum*String functions. >>> >>> FromString failure will report this error for value 'zzzz' >>> >>> invalid argument: Unknown 'domain type' value 'zzzz' >>> >>> ToString failure will report this error for unknown type=83 >>> >>> internal error: Unknown 'domain type' internal value '83' >>> >>> However we disable the error reporting for now. It should only be >>> enabled when we decide to begin dropping duplicate error reporting. >> >> Why don't we *enable* error reporting now, but only if the label >> string is non-NULL. Then just change your second patch to pass >> NULL everywhere. We can then incrementally replace the NULL with >> a real message at the same time as we update each caller to drop >> the existing error reporting. >> >> This avoids any point in time having double reporting. >> > > Yes but this hits the case I mentioned in the cover letter: until the > code base is converted it will be easier for new code to neglect to > raise virReportError when the enum hasn't been converted yet, and IMO > reporting no error is much worse than double reporting. > > If you still think it's the way to go I'll rework it. But then maybe we > need to think more about the strategy and timing of how we plan to > convert the code, do we shoot for doing it in a single release window? Given that the conversion work is essentially just a case of deleting 100's of calls to virReportError, it should be largely mechanical. So its reasonable to do it all in one release cycle. Assuming we get it done in a single cycle, I'm not concerned if we have a few temporary mistakes where we don't report any error.