Re: [PATCH v4 00/45] Less clumsy error checking
Patchew URL: https://patchew.org/QEMU/20200707160613.848843-1-arm...@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20200707160613.848843-1-arm...@redhat.com Subject: [PATCH v4 00/45] Less clumsy error checking === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/159413975752.169116.5808968580649255382.st...@bahia.lan -> patchew/159413975752.169116.5808968580649255382.st...@bahia.lan * [new tag] patchew/1594140062-23522-1-git-send-email-aleksandar.qemu.de...@gmail.com -> patchew/1594140062-23522-1-git-send-email-aleksandar.qemu.de...@gmail.com * [new tag] patchew/20200707163504.194740-1-kw...@redhat.com -> patchew/20200707163504.194740-1-kw...@redhat.com Switched to a new branch 'test' 5dad863 hmp: Ignore Error objects where the return value suffices 99fa3f2 qdev: Ignore Error objects where the return value suffices 6ee97c0 qemu-img: Ignore Error objects where the return value suffices 677256f error: Avoid error_propagate() after migrate_add_blocker() 974c52b qapi: Purge error_propagate() from QAPI core b664ca1 qapi: Smooth visitor error checking in generated code 58529e8 qapi: Smooth another visitor error checking pattern 8761cbd block/parallels: Simplify parallels_open() after previous commit 6821adb error: Reduce unnecessary error propagation 8ed0efc error: Eliminate error_propagate() manually 9f8cd51 error: Eliminate error_propagate() with Coccinelle, part 2 4516a2b error: Eliminate error_propagate() with Coccinelle, part 1 e776a9c error: Avoid unnecessary error_propagate() after error_setg() 97a8ae7 qdev: Use returned bool to check for failure, Coccinelle part b489fef qdev: Make functions taking Error ** return bool, not void c5651e9 qom: Make functions taking Error ** return bool, not 0/-1 1e9a1c3 qom: Use returned bool to check for failure, manual part b158de6 qom: Use returned bool to check for failure, Coccinelle part 911dbb4 qom: Make functions taking Error ** return bool, not void 94c4ad1 qom: Put name parameter before value / visitor parameter a65eadc qom: Use return values to check for error where that's simpler aff3f41 qom: Don't handle impossible object_property_get_link() failure f171d59 qom: Crash more nicely on object_property_get_link() failure d1d7b14 qom: Rename qdev_get_type() to object_get_type() 54e907c qom: Use error_reportf_err() instead of g_printerr() in examples aa402c4 s390x/pci: Fix harmless mistake in zpci's property fid's setter e3a01f0 qapi: Use returned bool to check for failure, manual part ee2a4a7 qapi: Use returned bool to check for failure, Coccinelle part d78fd21 qapi: Make visitor functions taking Error ** return bool, not void 45cc0d8 hmp: Eliminate a variable in hmp_migrate_set_parameter() 18fa30f block: Avoid error accumulation in bdrv_img_create() d5fa417 qemu-option: Use returned bool to check for failure 35a4f99 qemu-option: Make functions taking Error ** return bool, not void 9fcbf0b qemu-option: Replace opt_set() by cleaner opt_validate() 09fdc0e qemu-option: Factor out helper opt_create() f2370fc qemu-option: Simplify around find_default_by_name() e8d48f4 qemu-option: Factor out helper find_default_by_name() d9cd2e4 qemu-option: Make uses of find_desc_by_name() more similar 30d78ea qemu-option: Check return value instead of @err where convenient c670e7c virtio-crypto-pci: Tidy up virtio_crypto_pci_realize() 22252c6 macio: Tidy up error handling in macio_newworld_realize() 8c1b98a qdev: Use returned bool to check for qdev_realize() etc. failure 0ce error: Document Error API usage rules 2ae4ce8 error: Improve error.h's big comment 8d27f64 error: Fix examples in error.h's big comment === OUTPUT BEGIN === 1/45 Checking commit 8d27f64734f9 (error: Fix examples in error.h's big comment) ERROR: Error messages should not contain newlines #25: FILE: include/qapi/error.h:27: + * error_setg(, "invalid quark\n" // WRONG! total: 1 errors, 0 warnings, 40 lines checked Patch 1/45 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 2/45 Checking commit 2ae4ce84c712 (error: Improve error.h's big comment) 3/45 Checking commit 0ce2abc0 (error: Document Error API usage rules) 4/45 Checking commit 8c1b98ac618a (qdev: Use returned bool to check for qdev_realize() etc. failure) 5/45 Checking commit 22252c6387fd (macio: Tidy up error handling in macio_newworld_realize()) 6/45 Checking commit c670e7c4698a (virtio-crypto-pci: Tidy up virtio_crypto_pci_realize()) 7/45 Checking commit 30d78eae64ac (qemu-opti
Re: [PATCH v4 00/45] Less clumsy error checking
Neglected to mention: code changes are limited to resolving rebase conflicts in PATCH 26. v4 is about comments, mostly to reduce churn when combined with Vladimir's auto propagation work.
[PATCH v4 00/45] Less clumsy error checking
When the Error API was created, we adopted the (unwritten) rule to return void when the function returns no useful value on success, unlike GError, which recommends to return true on success and false on error then. When a function returns a distinct error value, say false, a checked call that passes the error up looks like if (!frobnicate(..., errp)) { handle the error... } When it returns void, we need Error *err = NULL; frobnicate(..., ); if (err) { handle the error... error_propagate(errp, err); } Not only is this more verbose, it also creates an Error object even when @errp is null, _abort or _fatal. People got tired of the additional boilerplate, and started to ignore the unwritten rule. The result is confusion among developers about the preferred usage. This series adopts the GError rule (in writing), and updates a substantial amount of code to honor the rule. Cuts the number of error_propagate() calls nearly by half. The diffstat speaks for itself. Also available from my public repository https://repo.or.cz/qemu/armbru.git on branch error-smooth. v4: * Rebased * PATCH 01: Don't show errp instead of , it's premature; leave it to Vladimir's series. Fix the error_propagate_prepend() pileup example. Update commit message accordingly. R-bys kept, hope that's not too bold. * PATCH 02: New. * PATCH 03: Structure the big comment with headlines. Explain the @errp usage restrictions more clearly. Update examples to prefer checking the return value over checking @err. Update commit message accordingly. R-bys kept, hope that's not too bold. * PATCH 07: Tweak commit message [Greg] * Left for later: followup to fix nearby typos and such [Eric] v3: * Rebased * Fix patch ordering: old PATCH 19 becomes PATCH 37 * PATCH 03+13+25+27: Avoid long lines in Coccinelle script [Eric] * PATCH 14: Fix commit message typo [Eric] * PATCH 16: Unbreak opts_start_list(), qapi_clone_start_list(), qapi_clone_start_alternate() [Vladimir] * PATCH 24: Unbreak object_set_link_property() [Vladimir] * PATCH 25+33+35: Move unrelated hunks from 25 to 33 and 35 [Vladimir], tweak line breaks * PATCH 32: Have commit message point out the unnecessary error_propagate() will be eliminated shortly [Eric] * PATCH 33: Fix commit message typo [Eric] * PATCH 35: Delete comment along with the assertion [Eric] * Left for later: followup to fix nearby typos and such [Eric] v2: * Rebased * Coccinelle scripts reworked, patches sliced and diced for reviewability: Old PATCH 03+17+23-24+35+38-39+42 split into "Use returned bool to check for failure" parts [PATCH 03+13+17-18+25+28-29+42], and "Eliminate error_propagate()" parts. The latter combined with old PATCH 06-07+18-19+29+43 are now [PATCH 33-37]. The end result is almost identical. Some R-bys dropped; sorry! * Drop variables as they become unused [Vladimir] * PATCH 01: Comment typos fixed [Greg] * PATCH 09: Simplify further by cutting out variables [Eric] * PATCH 11: opt_set() renamed to opt_validate(), redundant parameter dropped [Vladimir], R-by kept * PATCH 16: Comment typos fixed, indentation tidied up [Eric] * PATCH 23: Assertion dropped [Eric], incorrect hunk backed out * PATCH 26: Regenerated after rebase; note additional Coccinelle hiccup in the commit message * PATCH 27: Indentation tiedied up [Eric] * Left for later: followup to fix nearby typos and such [Eric] Markus Armbruster (45): error: Fix examples in error.h's big comment error: Improve error.h's big comment error: Document Error API usage rules qdev: Use returned bool to check for qdev_realize() etc. failure macio: Tidy up error handling in macio_newworld_realize() virtio-crypto-pci: Tidy up virtio_crypto_pci_realize() qemu-option: Check return value instead of @err where convenient qemu-option: Make uses of find_desc_by_name() more similar qemu-option: Factor out helper find_default_by_name() qemu-option: Simplify around find_default_by_name() qemu-option: Factor out helper opt_create() qemu-option: Replace opt_set() by cleaner opt_validate() qemu-option: Make functions taking Error ** return bool, not void qemu-option: Use returned bool to check for failure block: Avoid error accumulation in bdrv_img_create() hmp: Eliminate a variable in hmp_migrate_set_parameter() qapi: Make visitor functions taking Error ** return bool, not void qapi: Use returned bool to check for failure, Coccinelle part qapi: Use returned bool to check for failure, manual part s390x/pci: Fix harmless mistake in zpci's property fid's setter qom: Use error_reportf_err() instead of g_printerr() in examples qom: Rename qdev_get_type() to object_get_type() qom: Crash more nicely on object_property_get_link() failure qom: Don't handle impossible object_property_get_link() failure qom: Use return values to check for error where that's simpler qom: Put name parameter before value / visitor parameter qom: Make