Message ID | 20180809213528.14738-1-mreitz@redhat.com (mailing list archive) |
---|---|
Headers | show |
Series | block: Fix some filename generation issues | expand |
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20180809213528.14738-1-mreitz@redhat.com Subject: [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 4d2a6801e3 iotests: Test json:{} filenames of internal BDSs 92861b0d23 block: BDS options may lack the "driver" option 5e539a99ee block/null: Generate filename even with latency-ns 3011e18097 block/curl: Implement bdrv_refresh_filename() c3d3ba04c4 block/curl: Harmonize option defaults 0f00168fdc block/nvme: Fix bdrv_refresh_filename() 10fd7a823b block: Do not copy exact_filename from format file 1705480785 block: Purify .bdrv_refresh_filename() d5face8a37 block: Generically refresh runtime options d83b993208 block: Add BlockDriver.bdrv_gather_child_options 38384107dc block: Add strong_runtime_opts to BlockDriver b9b3310f5d iotests: Add quorum case to test 110 292e6e5c96 block: Use bdrv_dirname() for relative filenames f1fd748aca block/nfs: Implement bdrv_dirname() 8dfbf896b6 block/nbd: Make bdrv_dirname() return NULL a5187853f6 quorum: Make bdrv_dirname() return NULL 4f62d050cd blkverify: Make bdrv_dirname() return NULL f4ae7225b0 block: Add bdrv_dirname() 9b5ad84b84 block: Fix bdrv_find_backing_image() 324ade1026 block: Add bdrv_make_absolute_filename() 68def7295f block: bdrv_get_full_backing_filename's ret. val. e5e55fcbb3 block: bdrv_get_full_backing_filename_from_...'s ret. val. b434ec5b34 block: Make path_combine() return the path a3542a966b iotests: Add test for backing file overrides 6affaa70a0 iotests.py: Add node_info() 13d2e98fe3 iotests.py: Add filter_imgfmt() a46abb22e3 block: Respect backing bs in bdrv_refresh_filename 8b0cd321f7 block: Add BDS.auto_backing_file 0b9c999e16 block: Skip implicit nodes for filename info b4b7a10cc2 block: Use children list in bdrv_refresh_filename 92210c34b4 block: Use bdrv_refresh_filename() to pull === OUTPUT BEGIN === Checking PATCH 1/31: block: Use bdrv_refresh_filename() to pull... Checking PATCH 2/31: block: Use children list in bdrv_refresh_filename... Checking PATCH 3/31: block: Skip implicit nodes for filename info... Checking PATCH 4/31: block: Add BDS.auto_backing_file... Checking PATCH 5/31: block: Respect backing bs in bdrv_refresh_filename... ERROR: return is not a function, parentheses are not required #46: FILE: block.c:5192: + return (bs->auto_backing_file[0] != '\0'); total: 1 errors, 0 warnings, 136 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 6/31: iotests.py: Add filter_imgfmt()... Checking PATCH 7/31: iotests.py: Add node_info()... Checking PATCH 8/31: iotests: Add test for backing file overrides... WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #10: new file mode 100755 total: 0 errors, 1 warnings, 323 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 9/31: block: Make path_combine() return the path... Checking PATCH 10/31: block: bdrv_get_full_backing_filename_from_...'s ret. val.... Checking PATCH 11/31: block: bdrv_get_full_backing_filename's ret. val.... Checking PATCH 12/31: block: Add bdrv_make_absolute_filename()... Checking PATCH 13/31: block: Fix bdrv_find_backing_image()... Checking PATCH 14/31: block: Add bdrv_dirname()... Checking PATCH 15/31: blkverify: Make bdrv_dirname() return NULL... Checking PATCH 16/31: quorum: Make bdrv_dirname() return NULL... Checking PATCH 17/31: block/nbd: Make bdrv_dirname() return NULL... Checking PATCH 18/31: block/nfs: Implement bdrv_dirname()... Checking PATCH 19/31: block: Use bdrv_dirname() for relative filenames... Checking PATCH 20/31: iotests: Add quorum case to test 110... Checking PATCH 21/31: block: Add strong_runtime_opts to BlockDriver... Checking PATCH 22/31: block: Add BlockDriver.bdrv_gather_child_options... Checking PATCH 23/31: block: Generically refresh runtime options... Checking PATCH 24/31: block: Purify .bdrv_refresh_filename()... Checking PATCH 25/31: block: Do not copy exact_filename from format file... Checking PATCH 26/31: block/nvme: Fix bdrv_refresh_filename()... Checking PATCH 27/31: block/curl: Harmonize option defaults... Checking PATCH 28/31: block/curl: Implement bdrv_refresh_filename()... Checking PATCH 29/31: block/null: Generate filename even with latency-ns... Checking PATCH 30/31: block: BDS options may lack the "driver" option... Checking PATCH 31/31: iotests: Test json:{} filenames of internal BDSs... WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #10: new file mode 100755 total: 0 errors, 1 warnings, 164 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On 2018-08-16 08:02, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > >> On 2018-08-15 05:43, no-reply@patchew.org wrote: >>> Hi, >>> >>> This series seems to have some coding style problems. See output below for >>> more information: >> >> [...] >> >>> === OUTPUT BEGIN === >>> Checking PATCH 1/31: block: Use bdrv_refresh_filename() to pull... >>> Checking PATCH 2/31: block: Use children list in bdrv_refresh_filename... >>> Checking PATCH 3/31: block: Skip implicit nodes for filename info... >>> Checking PATCH 4/31: block: Add BDS.auto_backing_file... >>> Checking PATCH 5/31: block: Respect backing bs in bdrv_refresh_filename... >>> ERROR: return is not a function, parentheses are not required >>> #46: FILE: block.c:5192: >>> + return (bs->auto_backing_file[0] != '\0'); >>> >>> total: 1 errors, 0 warnings, 136 lines checked >> >> Sure, but I'd hate you personally if you omitted them. > > @@ > expression E; > @@ > - return (E); > + return E; > > You're welcome! Well, I really don't like not putting parenthesis when returning something that is not an identifier or a function call. In fact, before I drop the parentheses, I'd drop the "!= '\0'", because that is optional, too. (And then I could drop the parentheses anyway.) Or write it with an explicit if-else (return true - return false). Max
Am 09.08.2018 um 23:34 hat Max Reitz geschrieben: > Once more, I’ll spare both me and you another iteration of the cover > letter, so see here: > > http://lists.nongnu.org/archive/html/qemu-block/2017-09/msg01030.html > > (Although this series no longer includes a @base-directory option.) > > In regards to the last version, the biggest change is that I dropped > backing_overridden and instead try to compare the filename from the > image header with the filename of the actual backing BDS to find out > whether the backing file has been overridden. > > In order that this doesn’t break whenever the header contains a slightly > unusual (“non-canonical”) backing filename (e.g. “file:foo.qcow2” or > “nbd:localhost:10809” instead of “nbd://localhost:10809”, i.e. something > different from what bdrv_refresh_filename() would generate), when the > reference filename in the BDS (auto_backing_file) is used to open the > backing file, it is updated from the backing BDS's resulting filename. This doesn't seem to apply cleanly any more. Kevin
On 10.09.18 17:18, Kevin Wolf wrote: > Am 09.08.2018 um 23:34 hat Max Reitz geschrieben: >> Once more, I’ll spare both me and you another iteration of the cover >> letter, so see here: >> >> http://lists.nongnu.org/archive/html/qemu-block/2017-09/msg01030.html >> >> (Although this series no longer includes a @base-directory option.) >> >> In regards to the last version, the biggest change is that I dropped >> backing_overridden and instead try to compare the filename from the >> image header with the filename of the actual backing BDS to find out >> whether the backing file has been overridden. >> >> In order that this doesn’t break whenever the header contains a slightly >> unusual (“non-canonical”) backing filename (e.g. “file:foo.qcow2” or >> “nbd:localhost:10809” instead of “nbd://localhost:10809”, i.e. something >> different from what bdrv_refresh_filename() would generate), when the >> reference filename in the BDS (auto_backing_file) is used to open the >> backing file, it is updated from the backing BDS's resulting filename. > > This doesn't seem to apply cleanly any more. I know, but is that a "Please send your current v11 to the list"? Max
Am 10.09.2018 um 18:51 hat Max Reitz geschrieben: > On 10.09.18 17:18, Kevin Wolf wrote: > > Am 09.08.2018 um 23:34 hat Max Reitz geschrieben: > >> Once more, I’ll spare both me and you another iteration of the cover > >> letter, so see here: > >> > >> http://lists.nongnu.org/archive/html/qemu-block/2017-09/msg01030.html > >> > >> (Although this series no longer includes a @base-directory option.) > >> > >> In regards to the last version, the biggest change is that I dropped > >> backing_overridden and instead try to compare the filename from the > >> image header with the filename of the actual backing BDS to find out > >> whether the backing file has been overridden. > >> > >> In order that this doesn’t break whenever the header contains a slightly > >> unusual (“non-canonical”) backing filename (e.g. “file:foo.qcow2” or > >> “nbd:localhost:10809” instead of “nbd://localhost:10809”, i.e. something > >> different from what bdrv_refresh_filename() would generate), when the > >> reference filename in the BDS (auto_backing_file) is used to open the > >> backing file, it is updated from the backing BDS's resulting filename. > > > > This doesn't seem to apply cleanly any more. > > I know, but is that a "Please send your current v11 to the list"? Depends on how urgently you want me to review this. I don't like reviewing patches directly from emails, without being able to apply them. Or maybe if you just can give me the commit ID this is based on? I wouldn't mind applying it to an old tree as long as I can apply it somewhere. Kevin