Re: [libvirt] [PATCH 3/4] util: Add 'label' field to VIR_ENUM_IMPL
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?
The NULL idea is a good one anyway because at least some places like the
Tristate functions we likely won't want to raise errors, but those may
need to be treated differently anyways.
Thanks,
Cole