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.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
