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

[Peter Krempa]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190417/0900642d/attachment-0001.sig>