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

[Cole Robinson]

On 4/17/19 3:29 AM, Peter Krempa wrote:
> On Tue, Apr 16, 2019 at 12:02:41 -0400, Cole Robinson wrote:
>> 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
> 
> We will still end up in a half covered state if you don't make the
> second argument mandatory and fix all callers. This way you still get a
> majority of enum declarations which still pass NULL as 3/3 fixes exactly
> one of them.
> 
>> 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.
> 
> Also exactly the same is going to happen here as well. Caller of the
> From/ToString function needs to check whether the second argument in
> VIR_ENUM_DECL is NULL or not.
> 
> This can be avoided only by refactoring everything. Any other solution
> will get forgotten eventually and stay in half finished state forever.
> 

I think we are in agreement then. This NULL approach has all the same
problems with the RFE approach basically, it lends itself to an
incremental approach. The main difference with this approach is that it
still gives us an easy option to splice in strings en mass if we want.

>> 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.
> 
> There is also the problem that in some cases the pre-fabricated error
> string may not be desirable. An example is virStorageType.
> 
> Due to historical reasons the storage backend for a disk is an attribute
> of the <disk> xml whereas e.g. backingStore has somewhat saner design.
> 
> For <disk> we report "unknown disk type %s", for disk in side of
> <snapshot> "unknown disk snapshot type" is used and for <backingStore>
> and all other uses we have "unknown storage source" type.
> 
> In this case we probably could go with the last option but I can see
> that there will be instances where it will not be possible.
> 
> Thus it seems that we also need "Quiet" versions of the functions so
> that in case when a custom error is necessary it can be used without
> double-reporting an error.
> 

Yes I've had similar thoughts. Another major one is virTristate*, the
error reporting would give virtually no context. I figure for that case
we could implement the String functions manually, but require
virTristateTo/From callers to pass in a label field. Or maybe have
VIR_ENUM_IMPL_QUIET which generates only To/FromStringQuiet functions so
callers won't get confused.

So broadly I think the options are

- Flip the switch. Double error reporting until we remove now redundant
calls. Worse error reporting in some cases like tristate and
virstoragetype without special consideration. No or less issues with
having half converted codebase. IMO Easier to patch out the redundant
calls and easier to review the removals because we can do it per file
rather than per enum usage which might be spread across multiple files.

- Do it incrementally: will force us to consider each case individually
resulting in better overall error reporting. Until codebase is
converted, possible dev confusion and risk of new code neglecting to
raise an error. IMO the total dev and reviewer time is likely to be
significantly higher

I definitely favor 'flip the switch' mostly because I think it will get
this done the quickest, and once it's in git it distributes the load of
working out the kinks to the whole dev team. Depending on uptake the
incremental approach might never get finished, it's not clear. But
beyond that I'm not tied to any specific naming or method so I'm open to
ideas.

If consensus is to go for the incremental approach then I will support that

Thanks,
Cole