[libvirt] [PATCH v2 2/3] util: enum: Add NULL 'label' arg VIR_ENUM_IMPL calls

[Cole Robinson]

On 4/16/19 5:11 AM, Michal Privoznik wrote:
> On 4/16/19 8:32 AM, Peter Krempa wrote:
>> On Mon, Apr 15, 2019 at 17:26:41 -0400, Cole Robinson wrote:
>>> Allow passing in a 'label' string for raising errors from
>>> ToString/FromString calls. Adjust all VIR_ENUM_IMPL calls to
>>> pass in NULL to disable error reporting. We will add strings at
>>> a later time.
>>
>> I think that rather than changing every single VIR_ENUM_IMPL you should
>> rather add VIR_ENUM_IMPL_TYPE or VIR_ENUM_IMPL_MSG or something like
>> that which will allow to use the type string.
>>

This was the approach I took in the original RFC:

https://www.redhat.com/archives/libvir-list/2018-July/msg01815.html

I switched approaches for reasons I laid out in the last cover letter:

https://www.redhat.com/archives/libvir-list/2019-April/msg00589.html

Summary: I thought the original approach would make it more likely that
we would end up in a half converted state. Then devs would need need to
check whether use of a preexisting enum in new code is converted to
raise an error or not, increasing the likely hood we accidentally double
raise an error, or worse forget to raise an error when it's required.

The v1 approach added error strings to all labels with the intent of
turning on error reporting at a later time. Then the code would be fully
converted, and we would just need to strip out the double error
reporting which is the lesser of two evils than failing to raise an
error IMO. Dan suggested the NULL approach and incremental conversion
which is this v2.

I still prefer the v1 approach (error strings all added, then turn on
error reporting later). It will avoid the risk of new code forgetting to
raise an error, worst case is double error reporting which is not that
bad IMO. Once we turn on error reporting we can patch out
ToString/FromString errors by whole file rather than individual enum,
which will generate less patches that will be both easier to write and
easier to review (no searching around the code to see if someone missed
an enum usage). It front loads the interesting review part of
determining the string labels rather than sprinkling them over
potentially 200+ enum conversion patches.

So I'm interested in both your thoughts on all the above

>> That way it will also be far simpler to identify the places which need
>> fixing.
> 

IMO it's similar effort to identify places that need fixing: its grep of
'VIR_ENUM_IMPL_TYPE' vs 'VIR_ENUM_IMPL.*NULL'.


> Yes, that sounds like a better idea. For instance:
> 
> VIR_ENUM_IMPL_TYPE(virDomainVirt,
>                    _("invalid domain type %s"),

One of the benefits of centralizing error reporting is that we can reuse
a single translated string with a common format. Requiring translation
of every enum string is a waste IMO. And I don't think the label part
should be translatable as it is largely referencing XML element names,
or some API bit which is english only. I explained a bit more here:

https://www.redhat.com/archives/libvir-list/2019-April/msg00814.html

Thanks,
Cole