Re: [libvirt] [PATCH v2 2/3] util: enum: Add NULL 'label' arg VIR_ENUM_IMPL calls
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.
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.
Attachments:
- signature.asc (application/pgp-signature — 833 bytes)