On 9/18/19 12:46 PM, Vladimir Sementsov-Ogievskiy wrote: >>> +/* >>> + * Third variant: >>> + * Pros: >>> + * - simpler movement for functions which don't have local_err yet >>> + * the only thing to do is to call one macro at function start. >>> + * This extremely simplifies Greg's series >>> + * Cons: >>> + * - looks like errp shadowing.. Still seems safe. >>> + * - must be after all definitions of local variables and before any >>> + * code. >> >> Why? I see no reason why it can't be hoisted earlier than other >> declarations, and the only reason to not sink it after earlier code that >> doesn't touch errp would be our coding standards that frowns on >> declaration after code. > > Hmm, I thought compiler would warn about mixing code and definitions. > Seems that gcc don't care, so it's OK.
C89 required all definitions before code, but that's historical. Meanwhile, we require a compiler that supports C99 as well as at least the __attribute__((cleanup)) extension (gcc and clang qualify, nothing else really does, but no one has been complaining). And C99 requires compiler support for intermixing definitions (in part because c++ did it first, then gcc allowed that as an extension in C89); so it's been permissible to intermix code and declarations for more than 20 years now. The only reason we don't do it more is because of habits and aesthetics, rather than necessity. >> >> This is actually quite cool. > > I glad to see that you like my favorite variant) > >> And if you get rid of your insistence that >> it must occur after other variable declarations, you could instead >> easily automate that any function that has a parameter 'Error **errp' >> then has a MAKE_ERRP_SAFE(errp); as the first line of its function body >> (that becomes something that you could grep for, rather than having to >> use the smarts of coccinelle). >> >> Or if we want to enforce consistency on the parameter naming, even go with: >> >> #define MAKE_ERRP_SAFE() \ >> g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = errp}; \ >> errp = &__auto_errp_prop.local_err >> > > I am for So now to wait for other comments. >> >>> + */ >>> + MAKE_ERRP_SAFE(errp); >>> + >>> if (filename) { >>> ret = qemu_gluster_parse_uri(gconf, filename); >>> if (ret < 0) { >>> >> >> This is sweet - you can safely use '*errp' in the rest of the function, >> without having to remember a second error name - while the caller can >> still pass NULL or error_abort as desired. >> >> And I still think we can probably get Coccinelle to help make the >> conversions, both of using this macro in any function that has an Error >> **errp parameter, as well as getting rid of local_err declarations and >> error_propagate() calls rendered redundant once this macro is used. >> > > Thanks! And sorry for dirty draft. It was titled RFC, after all :) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org