On Wed, Apr 01, 2020 at 11:02:11AM +0200, Markus Armbruster wrote: > QEMU's Error was patterned after GLib's GError. Differences include: > > * &error_fatal, &error_abort for convenience
I think this doesn't really need to exist, and is an artifact of the later point "return values" where we commonly make methds return void. If we adopted a non-void return value, then these are no longer so compelling. Consider if we didn't have &error_fatal right now, then we would need to Error *local_err = NULL; qemu_boot_set(boot_once, &local_err) if (*local_err) abort(); This is tedious, so we invented &error_abort to make our lives better qemu_boot_set(boot_once, &error_abort) If we had a "bool" return value though, we would probably have just ended up doing: assert(qemu_boot_set(boot_once, NULL)); or if (!qemu_boot_set(boot_once, NULL)) abort() and would never have invented &error_fatal. > * Distinguishing different errors > > Where Error has ErrorClass, GError has Gquark domain, gint code. Use > of ErrorClass other than ERROR_CLASS_GENERIC_ERROR is strongly > discouraged. When we need callers to distinguish errors, we return > suitable error codes separately. The GQuark is just a static string, and in most cases this ends up being defined per-file, or sometimes per functional group. So essentially you can consider it to approximately a source file in most cases. The code is a constant of some arbitrary type that is generally considered to be scoped within the context of the GQuark domain. > * Return value conventions > > Common: non-void functions return a distinct error value on failure > when such a value can be defined. Patterns: > > - Functions returning non-null pointers on success return null pointer > on failure. > > - Functions returning non-negative integers on success return a > negative error code on failure. > > Different: GLib discourages void functions, because these lead to > awkward error checking code. We have tons of them, and tons of > awkward error checking code: > > Error *err = NULL; > frobnicate(arg, &err); > if (err) { > ... recover ... > error_propagate(errp, err); > } Yeah, I really dislike this verbose style... > > instead of > > if (!frobnicate(arg, errp)) > ... recover ... > } ...so I've followed this style for any code I've written in QEMU where possible. > > Can also lead to pointless creation of Error objects. > > I consider this a design mistake. Can we still fix it? We have more > than 2000 void functions taking an Error ** parameter... Even if we don't do full conversion, we can at least encourage the simpler style - previously reviewers have told me to rewrite code to use the more verbose style, which I resisted. So at the very least setting the expectations for preferred style is useful. > Transforming code that receives and checks for errors with Coccinelle > shouldn't be hard. Transforming code that returns errors seems more > difficult. We need to transform explicit and implicit return to > either return true or return false, depending on what we did to the > @errp parameter on the way to the return. Hmm. Even if we only converted methods which are currently void, that would be a notable benefit I think. It is a shame we didn't just use GError from the start, but I guess its probably too late to consider changing that now. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|