On 9/19/19 10:50 AM, Daniel P. Berrangé wrote: > To get some slightly less made-up stats, I did some searching: > > - 4408 methods with an 'errp' parameter declared > > git grep 'Error \*\*errp'| wc -l > > - 696 methods with an 'Error *local' declared (what other names > do we use for this?) > > git grep 'Error \*local' | wc -l
Covers 'local' and 'local_err'. We also have: git grep 'Error \*err\b' | wc -l 553 So 1249 local errors. > > - 1243 methods with an 'errp' parameter which have void > return value (fuzzy number - my matching is quite crude) > > git grep 'Error \*\*errp'| grep -E '(^| )void' | wc -l > > - 11 methods using error_append_hint with a local_err > > git grep append_hint | grep local | wc -l > > > This suggests to me, that if we used the 'return 0 / return -1' convention > to indicate failure for the methods which currently return void, we could > potentially only have 11 cases where a local error object is genuinely > needed. > > If my numbers are at all accurate, I still believe we're better off > changing the return type and eliminating essentially all use of local > error variables, as void funcs/local error objects appear to be the > minority coding pattern. When relying on a return value, you do have to check whether a negative return value happens if and only if errp is set; that's something that's harder for code grepping to spot. I'm not opposed to that coding pattern, just pointing out that it requires some semantic analysis, while MAKE_ERRP_SAFE() coupled with checking *errp is easier to prove at a glance that the check for whether an error happened is 1:1 associated with whether an error is reported. > > >>> There are lots of neat things we could do with auto-cleanup functions we >>> I think we need to be wary of hiding too much cleverness behind macros >>> when doing so overall. >> >> The benefit of getting rid of the 'Error *local_err = NULL; ... >> error_propagate()' boilerplate is worth the cleverness, in my opinion, >> but especially if also accompanied by CI coverage that we abide by our >> new rules. > > At least we're both wanting to eliminate the local error + propagation. > The difference is whether we're genuinely eliminating it, or just hiding > it from view via auto-cleanup & macro magic > > Regards, > Daniel > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
