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