| 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
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. Changes in v10: - Patches 2, 21, 24: Accomodate for blklogwrites - Patch 5: Put bdrv_backing_overridden() into an own function, so I can use it in a follow-up series that tries to make bdrv_find_backing_image() work - Patch 7: Add an optional parameter to node_info() so the caller can choose whether this function shall return None if the node is not found, or fail the whole test [Berto] - Patch 8: Just changed the test number Backport-diff: Key: [----] : patches are identical [####] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/31:[----] [--] 'block: Use bdrv_refresh_filename() to pull' 002/31:[0003] [FC] 'block: Use children list in bdrv_refresh_filename' 003/31:[----] [--] 'block: Skip implicit nodes for filename info' 004/31:[----] [--] 'block: Add BDS.auto_backing_file' 005/31:[0020] [FC] 'block: Respect backing bs in bdrv_refresh_filename' 006/31:[----] [--] 'iotests.py: Add filter_imgfmt()' 007/31:[0007] [FC] 'iotests.py: Add node_info()' 008/31:[0002] [FC] 'iotests: Add test for backing file overrides' 009/31:[----] [--] 'block: Make path_combine() return the path' 010/31:[----] [--] 'block: bdrv_get_full_backing_filename_from_...'s ret. val.' 011/31:[----] [--] 'block: bdrv_get_full_backing_filename's ret. val.' 012/31:[----] [--] 'block: Add bdrv_make_absolute_filename()' 013/31:[----] [--] 'block: Fix bdrv_find_backing_image()' 014/31:[----] [--] 'block: Add bdrv_dirname()' 015/31:[----] [--] 'blkverify: Make bdrv_dirname() return NULL' 016/31:[----] [--] 'quorum: Make bdrv_dirname() return NULL' 017/31:[----] [--] 'block/nbd: Make bdrv_dirname() return NULL' 018/31:[----] [--] 'block/nfs: Implement bdrv_dirname()' 019/31:[----] [--] 'block: Use bdrv_dirname() for relative filenames' 020/31:[----] [--] 'iotests: Add quorum case to test 110' 021/31:[0008] [FC] 'block: Add strong_runtime_opts to BlockDriver' 022/31:[----] [--] 'block: Add BlockDriver.bdrv_gather_child_options' 023/31:[----] [--] 'block: Generically refresh runtime options' 024/31:[0023] [FC] 'block: Purify .bdrv_refresh_filename()' 025/31:[----] [--] 'block: Do not copy exact_filename from format file' 026/31:[----] [--] 'block/nvme: Fix bdrv_refresh_filename()' 027/31:[----] [--] 'block/curl: Harmonize option defaults' 028/31:[----] [--] 'block/curl: Implement bdrv_refresh_filename()' 029/31:[----] [--] 'block/null: Generate filename even with latency-ns' 030/31:[----] [--] 'block: BDS options may lack the "driver" option' 031/31:[----] [-C] 'iotests: Test json:{} filenames of internal BDSs' Max Reitz (31): block: Use bdrv_refresh_filename() to pull block: Use children list in bdrv_refresh_filename block: Skip implicit nodes for filename info block: Add BDS.auto_backing_file block: Respect backing bs in bdrv_refresh_filename iotests.py: Add filter_imgfmt() iotests.py: Add node_info() iotests: Add test for backing file overrides block: Make path_combine() return the path block: bdrv_get_full_backing_filename_from_...'s ret. val. block: bdrv_get_full_backing_filename's ret. val. block: Add bdrv_make_absolute_filename() block: Fix bdrv_find_backing_image() block: Add bdrv_dirname() blkverify: Make bdrv_dirname() return NULL quorum: Make bdrv_dirname() return NULL block/nbd: Make bdrv_dirname() return NULL block/nfs: Implement bdrv_dirname() block: Use bdrv_dirname() for relative filenames iotests: Add quorum case to test 110 block: Add strong_runtime_opts to BlockDriver block: Add BlockDriver.bdrv_gather_child_options block: Generically refresh runtime options block: Purify .bdrv_refresh_filename() block: Do not copy exact_filename from format file block/nvme: Fix bdrv_refresh_filename() block/curl: Harmonize option defaults block/curl: Implement bdrv_refresh_filename() block/null: Generate filename even with latency-ns block: BDS options may lack the "driver" option iotests: Test json:{} filenames of internal BDSs include/block/block.h | 16 +- include/block/block_int.h | 48 +++- block.c | 513 ++++++++++++++++++++++------------ block/blkdebug.c | 70 ++--- block/blklogwrites.c | 34 +-- block/blkverify.c | 29 +- block/commit.c | 3 +- block/crypto.c | 8 + block/curl.c | 55 +++- block/gluster.c | 19 ++ block/iscsi.c | 18 ++ block/mirror.c | 3 +- block/nbd.c | 46 +-- block/nfs.c | 54 ++-- block/null.c | 32 ++- block/nvme.c | 27 +- block/qapi.c | 16 +- block/qcow.c | 14 +- block/qcow2.c | 17 +- block/qed.c | 7 +- block/quorum.c | 71 +++-- block/raw-format.c | 11 +- block/rbd.c | 14 + block/replication.c | 10 +- block/sheepdog.c | 12 + block/ssh.c | 12 + block/throttle.c | 7 + block/vhdx-log.c | 1 + block/vmdk.c | 43 ++- block/vpc.c | 11 +- block/vvfat.c | 12 + block/vxhs.c | 11 + blockdev.c | 8 + qemu-img.c | 12 +- tests/qemu-iotests/051.out | 8 +- tests/qemu-iotests/051.pc.out | 8 +- tests/qemu-iotests/110 | 29 +- tests/qemu-iotests/110.out | 9 +- tests/qemu-iotests/224 | 139 +++++++++ tests/qemu-iotests/224.out | 18 ++ tests/qemu-iotests/228 | 235 ++++++++++++++++ tests/qemu-iotests/228.out | 84 ++++++ tests/qemu-iotests/group | 2 + tests/qemu-iotests/iotests.py | 13 + 44 files changed, 1393 insertions(+), 416 deletions(-) create mode 100755 tests/qemu-iotests/224 create mode 100644 tests/qemu-iotests/224.out create mode 100755 tests/qemu-iotests/228 create mode 100644 tests/qemu-iotests/228.out