Re: [RFC v2 02/10] Drop unused static function return values
On Wed, Aug 3, 2022 at 12:15 PM Richard W.M. Jones wrote: > If it helps to think about this, Coverity checks for consistency. > Across the whole code base, is the return value of a function used or > ignored consistently. You will see Coverity errors like: > > Error: CHECKED_RETURN (CWE-252): [#def37] > libnbd-1.12.5/fuse/operations.c:180: check_return: Calling "nbd_poll" > without checking return value (as is done elsewhere 5 out of 6 times). > libnbd-1.12.5/examples/aio-connect-read.c:96: example_checked: Example > 1: "nbd_poll(nbd, -1)" has its value checked in "nbd_poll(nbd, -1) == -1". > libnbd-1.12.5/examples/aio-connect-read.c:128: example_checked: Example > 2: "nbd_poll(nbd, -1)" has its value checked in "nbd_poll(nbd, -1) == -1". > libnbd-1.12.5/examples/strict-structured-reads.c:246: example_checked: > Example 3: "nbd_poll(nbd, -1)" has its value checked in "nbd_poll(nbd, -1) == > -1". > libnbd-1.12.5/ocaml/nbd-c.c:2599: example_assign: Example 4: Assigning: > "r" = return value from "nbd_poll(h, timeout)". > libnbd-1.12.5/ocaml/nbd-c.c:2602: example_checked: Example 4 (cont.): > "r" has its value checked in "r == -1". > libnbd-1.12.5/python/methods.c:2806: example_assign: Example 5: > Assigning: "ret" = return value from "nbd_poll(h, timeout)". > libnbd-1.12.5/python/methods.c:2808: example_checked: Example 5 > (cont.): "ret" has its value checked in "ret == -1". > # 178| /* Dispatch work while there are commands in flight. */ > # 179| while (thread->in_flight > 0) > # 180|-> nbd_poll (h, -1); > # 181| } > # 182| > > What it's saying is that in this code base, nbd_poll's return value > was checked by the caller 5 out of 6 times, but ignored here. (This > turned out to be a real bug which we fixed). > > It seems like the check implemented in your patch is: If the return > value is used 0 times anywhere in the code base, change the return > value to 'void'. Coverity would not flag this. > > Maybe a consistent use check is better? Note that the analyzer is currently limited to analyzing a single translation unit at a time, so we would only be able to implement a consistent use check for static functions (this is why the current "return-value-never-used" check only applies to static functions). It may be worthwhile exploring cross-translation unit analysis, although it may be difficult to accomplish while also avoiding reanalyzing the entire tree every time a single translation unit is modified. Alberto
Re: [RFC v2 02/10] Drop unused static function return values
On Wed, Aug 3, 2022 at 1:30 PM Peter Maydell wrote: > The problem with a patch like this is that it rolls up into a > single patch changes to the API of many functions in multiple > subsystems across the whole codebase. Some of those changes > might be right; some might be wrong. No single person is going > to be in a position to review the whole lot, and a +248-403 > patch email makes it very unwieldy to try to discuss. > > If you want to propose some of these I think you need to: > * split it out so that you're only suggesting changes in >one subsystem at a time > * look at the places you are suggesting changes, to see if >the correct answer is actually "add the missing error >check in the caller(s)" > * not change places that are following standard API patterns >like "return bool and have an Error** argument" Sounds good. For now, I'll limit the changes to a few representative cases e.g. in the block layer, since this patch is mostly intended as a demonstration of the type of issue that the check catches. Once there is agreement on the semantics for the check, I'll probably send a separate tree-wide series with per-subsystem patches. Thanks, Alberto
Re: [RFC v2 00/10] Introduce an extensible static analyzer
On Thu, Aug 4, 2022 at 12:44 PM Marc-André Lureau wrote: > Hi > > Great work so far! This seems easier to hack than my attempt to use > clang-tidy to write some qemu checks > (https://github.com/elmarco/clang-tools-extra) > > The code seems quite generic, I wonder if such a tool in python wasn't > already developed (I couldn't find it easily searching on github). > > Why not make it standalone from qemu? Similar to > https://gitlab.com/qemu-project/python-qemu-qmp, you could have your > own release management, issue tracker, code formatting, license, CI > etc. (you should add copyright header in each file, at least that's > pretty much required in qemu nowadays). You could also have the > qemu-specific checks there imho (clang-tidy has google & llvm specific > checks too) This is an interesting idea. Indeed, the analyzer is essentially a more easily extensible, Python version of clang-tidy (without the big built-in library of checks). I think I'll continue working on it embedded in QEMU for now, mostly because it depends on some aspects of the build system, and gradually generalize it to a point where it could be made into a standalone thing. > It would be nice to write some docs, in docs/devel/testing.rst and > some new meson/ninja/make targets to run the checks directly from a > build tree. Sounds good, I'll work on it. > On fc36, I had several dependencies I needed to install manually (imho > they should have been pulled by python3-clang), but more annoyingly I > got: > clang.cindex.LibclangError: libclang.so: cannot open shared object > file: No such file or directory. To provide a path to libclang use > Config.set_library_path() or Config.set_library_file(). > > clang-libs doesn't install libclang.so, I wonder why. I made a link > manually and it works, but it's probably incorrect. I'll try to open > issues for the clang packaging. That's strange. Thanks for looking into this. Alberto
Re: [PATCH v2 0/8] parallels: Refactor the code of images checks and fix a bug
On 11.08.2022 17:00, Alexander Ivanov wrote: Fix image inflation when offset in BAT is out of image. Replace whole BAT syncing by flushing only dirty blocks. Move all the checks outside the main check function in separate functions Use WITH_QEMU_LOCK_GUARD for more clean code. Alexander Ivanov (8): parallels: Out of image offset in BAT leads to image inflation parallels: Move BAT entry setting to a separate function parallels: Replace bdrv_co_pwrite_sync by bdrv_co_flush for BAT flushing parallels: Move check of unclean image to a separate function parallels: Move check of cluster outside image to a separate function parallels: Move check of leaks to a separate function parallels: Move statistic collection to a separate function parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD block/parallels.c | 188 -- 1 file changed, 132 insertions(+), 56 deletions(-) I believe that this series is ready to go once commit messages will be improved. Den
Re: [PATCH v2 8/8] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD
On 11.08.2022 17:00, Alexander Ivanov wrote:
Replace the way we use mutex in parallels_co_check() for more clean code.
I think that "cleaness" is the same, but new code would be just shorter ;)
or less error prone.
v2: Fix an incorrect usage of WITH_QEMU_LOCK_GUARD.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 26 --
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index d0364182bb..e124a8bb7d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -553,24 +553,22 @@ static int coroutine_fn
parallels_co_check(BlockDriverState *bs,
BDRVParallelsState *s = bs->opaque;
int ret;
-qemu_co_mutex_lock(>lock);
+WITH_QEMU_LOCK_GUARD(>lock) {
+parallels_check_unclean(bs, res, fix);
-parallels_check_unclean(bs, res, fix);
+ret = parallels_check_outside_image(bs, res, fix);
+if (ret < 0) {
+return ret;
+}
-ret = parallels_check_outside_image(bs, res, fix);
-if (ret < 0) {
-goto out;
-}
-
-ret = parallels_check_leak(bs, res, fix);
-if (ret < 0) {
-goto out;
-}
+ret = parallels_check_leak(bs, res, fix);
+if (ret < 0) {
+return ret;
+}
-parallels_collect_statistics(bs, res, fix);
+parallels_collect_statistics(bs, res, fix);
-out:
-qemu_co_mutex_unlock(>lock);
+}
ret = bdrv_co_flush(bs);
if (ret < 0) {
Re: [PATCH v2 7/8] parallels: Move statistic collection to a separate function
On 11.08.2022 17:00, Alexander Ivanov wrote:
v2: Move fragmentation counting code to this function too.
same note here about ChnageLog and motivation
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 54 +++
1 file changed, 31 insertions(+), 23 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 8737eadfb4..d0364182bb 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -518,48 +518,56 @@ static int parallels_check_leak(BlockDriverState *bs,
return 0;
}
-static int coroutine_fn parallels_co_check(BlockDriverState *bs,
- BdrvCheckResult *res,
- BdrvCheckMode fix)
+static void parallels_collect_statistics(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ BdrvCheckMode fix)
{
BDRVParallelsState *s = bs->opaque;
-int64_t prev_off;
-int ret;
+int64_t off, prev_off;
uint32_t i;
-
-qemu_co_mutex_lock(>lock);
-
-parallels_check_unclean(bs, res, fix);
-
-ret = parallels_check_outside_image(bs, res, fix);
-if (ret < 0) {
-goto out;
-}
-
-ret = parallels_check_leak(bs, res, fix);
-if (ret < 0) {
-goto out;
-}
-
res->bfi.total_clusters = s->bat_size;
res->bfi.compressed_clusters = 0; /* compression is not supported */
prev_off = 0;
for (i = 0; i < s->bat_size; i++) {
-int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+off = bat2sect(s, i) << BDRV_SECTOR_BITS;
if (off == 0) {
prev_off = 0;
continue;
}
-res->bfi.allocated_clusters++;
-
if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
res->bfi.fragmented_clusters++;
}
+
prev_off = off;
+res->bfi.allocated_clusters++;
}
+}
+
+static int coroutine_fn parallels_co_check(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ BdrvCheckMode fix)
+{
+BDRVParallelsState *s = bs->opaque;
+int ret;
+
+qemu_co_mutex_lock(>lock);
+
+parallels_check_unclean(bs, res, fix);
+
+ret = parallels_check_outside_image(bs, res, fix);
+if (ret < 0) {
+goto out;
+}
+
+ret = parallels_check_leak(bs, res, fix);
+if (ret < 0) {
+goto out;
+}
+
+parallels_collect_statistics(bs, res, fix);
out:
qemu_co_mutex_unlock(>lock);
Re: [PATCH v2 6/8] parallels: Move check of leaks to a separate function
On 11.08.2022 17:00, Alexander Ivanov wrote:
v2: No changes.
same notes about motivation, changelog as before
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 85 +--
1 file changed, 52 insertions(+), 33 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 12104ba5ad..8737eadfb4 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -469,14 +469,13 @@ static int parallels_check_outside_image(BlockDriverState
*bs,
return 0;
}
-static int coroutine_fn parallels_co_check(BlockDriverState *bs,
- BdrvCheckResult *res,
- BdrvCheckMode fix)
+static int parallels_check_leak(BlockDriverState *bs,
+BdrvCheckResult *res,
+BdrvCheckMode fix)
{
BDRVParallelsState *s = bs->opaque;
-int64_t size, prev_off, high_off;
-int ret;
-uint32_t i;
+int64_t size, off, high_off, count;
+int i, ret;
size = bdrv_getlength(bs->file->bs);
if (size < 0) {
@@ -484,41 +483,16 @@ static int coroutine_fn
parallels_co_check(BlockDriverState *bs,
return size;
}
-qemu_co_mutex_lock(>lock);
-
-parallels_check_unclean(bs, res, fix);
-
-ret = parallels_check_outside_image(bs, res, fix);
-if (ret < 0) {
-goto out;
-}
-
-res->bfi.total_clusters = s->bat_size;
-res->bfi.compressed_clusters = 0; /* compression is not supported */
-
high_off = 0;
-prev_off = 0;
for (i = 0; i < s->bat_size; i++) {
-int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
-if (off == 0) {
-prev_off = 0;
-continue;
-}
-
-res->bfi.allocated_clusters++;
+off = bat2sect(s, i) << BDRV_SECTOR_BITS;
if (off > high_off) {
high_off = off;
}
-
-if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
-res->bfi.fragmented_clusters++;
-}
-prev_off = off;
}
res->image_end_offset = high_off + s->cluster_size;
if (size > res->image_end_offset) {
-int64_t count;
count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
fprintf(stderr, "%s space leaked at the end of the image %" PRId64
"\n",
fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
@@ -536,11 +510,56 @@ static int coroutine_fn
parallels_co_check(BlockDriverState *bs,
if (ret < 0) {
error_report_err(local_err);
res->check_errors++;
-goto out;
+return ret;
}
res->leaks_fixed += count;
}
}
+return 0;
+}
+
+static int coroutine_fn parallels_co_check(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ BdrvCheckMode fix)
+{
+BDRVParallelsState *s = bs->opaque;
+int64_t prev_off;
+int ret;
+uint32_t i;
+
+
please remove extra empty line. This looks like an accident
+qemu_co_mutex_lock(>lock);
+
+parallels_check_unclean(bs, res, fix);
+
+ret = parallels_check_outside_image(bs, res, fix);
+if (ret < 0) {
+goto out;
+}
+
+ret = parallels_check_leak(bs, res, fix);
+if (ret < 0) {
+goto out;
+}
+
+res->bfi.total_clusters = s->bat_size;
+res->bfi.compressed_clusters = 0; /* compression is not supported */
+
+prev_off = 0;
+for (i = 0; i < s->bat_size; i++) {
+int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+if (off == 0) {
+prev_off = 0;
+continue;
+}
+
+res->bfi.allocated_clusters++;
+
+if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
+res->bfi.fragmented_clusters++;
+}
+prev_off = off;
+}
out:
qemu_co_mutex_unlock(>lock);
Re: [PATCH v2 5/8] parallels: Move check of cluster outside image to a separate function
On 11.08.2022 17:00, Alexander Ivanov wrote:
v2: Move unrelated helper parallels_set_bat_entry creation to
a separate patch.
same notes as for previous patch
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 48 ++-
1 file changed, 35 insertions(+), 13 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index c53b2810cf..12104ba5ad 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -439,6 +439,36 @@ static void parallels_check_unclean(BlockDriverState *bs,
}
}
+static int parallels_check_outside_image(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ BdrvCheckMode fix)
+{
+BDRVParallelsState *s = bs->opaque;
+uint32_t i;
+int64_t off, size;
+
+size = bdrv_getlength(bs->file->bs);
+if (size < 0) {
+res->check_errors++;
+return size;
+}
+
+for (i = 0; i < s->bat_size; i++) {
+off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+if (off > size) {
+fprintf(stderr, "%s cluster %u is outside image\n",
+fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+res->corruptions++;
+if (fix & BDRV_FIX_ERRORS) {
+parallels_set_bat_entry(s, i, 0);
+res->corruptions_fixed++;
+}
+}
+}
+
+return 0;
+}
+
static int coroutine_fn parallels_co_check(BlockDriverState *bs,
BdrvCheckResult *res,
BdrvCheckMode fix)
@@ -458,6 +488,11 @@ static int coroutine_fn
parallels_co_check(BlockDriverState *bs,
parallels_check_unclean(bs, res, fix);
+ret = parallels_check_outside_image(bs, res, fix);
+if (ret < 0) {
+goto out;
+}
+
res->bfi.total_clusters = s->bat_size;
res->bfi.compressed_clusters = 0; /* compression is not supported */
@@ -470,19 +505,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
continue;
}
-/* cluster outside the image */
-if (off > size) {
-fprintf(stderr, "%s cluster %u is outside image\n",
-fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
-res->corruptions++;
-if (fix & BDRV_FIX_ERRORS) {
-prev_off = 0;
-parallels_set_bat_entry(s, i, 0);
-res->corruptions_fixed++;
-continue;
-}
-}
-
res->bfi.allocated_clusters++;
if (off > high_off) {
high_off = off;
Re: [PATCH v2 4/8] parallels: Move check of unclean image to a separate function
On 11.08.2022 17:00, Alexander Ivanov wrote:
v2: Revert the condition with s->header_unclean.
same comment about change log as previously
And commit message misses motivation part, why we are
doing this rework. What is the goal of this change?
The code part is clean.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 31 +--
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 6879ea4597..c53b2810cf 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -419,6 +419,25 @@ static coroutine_fn int
parallels_co_readv(BlockDriverState *bs,
return ret;
}
+static void parallels_check_unclean(BlockDriverState *bs,
+BdrvCheckResult *res,
+BdrvCheckMode fix)
+{
+BDRVParallelsState *s = bs->opaque;
+
+if (!s->header_unclean) {
+return;
+}
+
+fprintf(stderr, "%s image was not closed correctly\n",
+fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
+res->corruptions++;
+if (fix & BDRV_FIX_ERRORS) {
+/* parallels_close will do the job right */
+res->corruptions_fixed++;
+s->header_unclean = false;
+}
+}
static int coroutine_fn parallels_co_check(BlockDriverState *bs,
BdrvCheckResult *res,
@@ -436,16 +455,8 @@ static int coroutine_fn
parallels_co_check(BlockDriverState *bs,
}
qemu_co_mutex_lock(>lock);
-if (s->header_unclean) {
-fprintf(stderr, "%s image was not closed correctly\n",
-fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
-res->corruptions++;
-if (fix & BDRV_FIX_ERRORS) {
-/* parallels_close will do the job right */
-res->corruptions_fixed++;
-s->header_unclean = false;
-}
-}
+
+parallels_check_unclean(bs, res, fix);
res->bfi.total_clusters = s->bat_size;
res->bfi.compressed_clusters = 0; /* compression is not supported */
Re: [PATCH v2 3/8] parallels: Replace bdrv_co_pwrite_sync by bdrv_co_flush for BAT flushing
Use generic infrastructure for BAT writing in parallels_co_check()
On 11.08.2022 17:00, Alexander Ivanov wrote:
It's too costly to write all the BAT to the disk. Let the flush function
write only dirty blocks.
Use parallels_set_bat_entry for setting a BAT entry and marking a relevant
block as dirty.
Move bdrv_co_flush call outside the locked area.
This is not correct too:
- with a lot of cases inside parallels_co_check, which will be split to
smaller functions, we would like to avoid writing BAT once each
stage is complete
Thus the comment should look like the following:
"BAT is written in the context of conventional operations over
the image inside bdrv_co_flush() when it calls
parallels_co_flush_to_os() callback. Thus we should not
modify BAT array directly, but call parallels_set_bat_entry()
helper and bdrv_co_flush() further on. After that there is no
need to manually write BAT and track its modification.
This makes code more generic and allows to split
parallels_set_bat_entry() for independent pieces."
v2: Patch order was changed so the replacement is done in parallels_co_check.
Now we use a helper to set BAT entry and mark the block dirty.
Same note about changelog as n other patches.
Side note. I like Linux kernel a lot and there is a good
guide in. Please look how commit message
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
May be you could spend some more time on commit
message.
With a better message:
Reviewed-by: Denis V. Lunev
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 19 +++
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 7f68f3cbc9..6879ea4597 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -428,7 +428,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState
*bs,
int64_t size, prev_off, high_off;
int ret;
uint32_t i;
-bool flush_bat = false;
size = bdrv_getlength(bs->file->bs);
if (size < 0) {
@@ -467,9 +466,8 @@ static int coroutine_fn parallels_co_check(BlockDriverState
*bs,
res->corruptions++;
if (fix & BDRV_FIX_ERRORS) {
prev_off = 0;
-s->bat_bitmap[i] = 0;
+parallels_set_bat_entry(s, i, 0);
res->corruptions_fixed++;
-flush_bat = true;
continue;
}
}
@@ -485,15 +483,6 @@ static int coroutine_fn
parallels_co_check(BlockDriverState *bs,
prev_off = off;
}
-ret = 0;
-if (flush_bat) {
-ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
-if (ret < 0) {
-res->check_errors++;
-goto out;
-}
-}
-
res->image_end_offset = high_off + s->cluster_size;
if (size > res->image_end_offset) {
int64_t count;
@@ -522,6 +511,12 @@ static int coroutine_fn
parallels_co_check(BlockDriverState *bs,
out:
qemu_co_mutex_unlock(>lock);
+
+ret = bdrv_co_flush(bs);
+if (ret < 0) {
+res->check_errors++;
+}
+
return ret;
}
Re: [PATCH v2 2/8] parallels: Move BAT entry setting to a separate function
On 11.08.2022 17:00, Alexander Ivanov wrote:
Will need to set BAT entry in multiple places.
Move the code of settings entries and marking relevant blocks dirty
to a separate helper parallels_set_bat_entry.
The comment and the patch text is ambiguous.
You say that we need to set BAT in multiple places
but patch changes one place only. I think that it would
be better to say like the following:
"parallels: create parallels_set_bat_entry_helper() to assign BAT value
This helper will be reused in next patches during parallels_co_check
rework to simplify its code"
v2: A new patch - a part of a splitted patch.
Same note about version tracking
With above taken into account:
Reviewed-by: Denis V. Lunev
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index a76cf9d993..7f68f3cbc9 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -165,6 +165,13 @@ static int64_t block_status(BDRVParallelsState *s, int64_t
sector_num,
return start_off;
}
+static void parallels_set_bat_entry(BDRVParallelsState *s,
+uint32_t index, uint32_t offset)
+{
+s->bat_bitmap[index] = offset;
+bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block,
1);
+}
+
static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, int *pnum)
{
@@ -250,10 +257,9 @@ static int64_t allocate_clusters(BlockDriverState *bs,
int64_t sector_num,
}
for (i = 0; i < to_allocate; i++) {
-s->bat_bitmap[idx + i] = cpu_to_le32(s->data_end / s->off_multiplier);
+parallels_set_bat_entry(s, idx + i,
+cpu_to_le32(s->data_end / s->off_multiplier));
s->data_end += s->tracks;
-bitmap_set(s->bat_dirty_bmap,
- bat_entry_off(idx + i) / s->bat_dirty_block, 1);
}
return bat2sect(s, idx) + sector_num % s->tracks;
Re: [PATCH v2 1/8] parallels: Out of image offset in BAT leads to image inflation
On 11.08.2022 17:00, Alexander Ivanov wrote:
When an image is opened, data_end field in BDRVParallelsState
is setted as the biggest offset in the BAT plus cluster size.
If there is a corrupted offset pointing outside the image,
the image size increase accordingly. It potentially leads
to attempts to create a file size of petabytes.
Set the data_end field with the original file size if the image
was opened for checking and repairing purposes or raise an error.
v2: No changes.
Changelog should be below ---
In that case it will not be merged.
There are a lot of typos/mistakes inside, I'd better use the comment
below.
"data_end field in BDRVParallelsState is set to the biggest offset present
in BAT. If this offset is outside of the image, any further write will
create
the cluster at this offset and/or the image will be truncated to this
offset on close. This is definitely not correct and should be fixed."
With this change:
Reviewed-by: Denis V. Lunev
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index a229c06f25..a76cf9d993 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
BDRVParallelsState *s = bs->opaque;
ParallelsHeader ph;
int ret, size, i;
+int64_t file_size;
QemuOpts *opts = NULL;
Error *local_err = NULL;
char *buf;
@@ -811,6 +812,22 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
}
+file_size = bdrv_getlength(bs->file->bs);
+if (file_size < 0) {
+goto fail;
+}
+
+file_size >>= BDRV_SECTOR_BITS;
+if (s->data_end > file_size) {
+if (flags & BDRV_O_CHECK) {
+s->data_end = file_size;
+} else {
+error_setg(errp, "parallels: Offset in BAT is out of image");
+ret = -EINVAL;
+goto fail;
+}
+}
+
if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
/* Image was not closed correctly. The check is mandatory */
s->header_unclean = true;
[PATCH] hw/nvme: remove param zoned.auto_transition
The intention of the Zoned Namespace Command Set Specification was
never to make an automatic zone transition optional.
Excerpt from the nvmexpress.org zns mailing list:
"""
A question came up internally on the differences between ZNS and ZAC/ZBC
that asked about when a controller should transitions a specific zone in
the Implicitly Opened state to Closed state.
For example, consider a ZNS SSD that supports a max of 20 active zones,
and a max of 10 open zones, which has the following actions occur:
First, the host writes to ten empty zones, thereby transitioning 10 zones
to the Implicitly Opened state.
Second, the host issues a write to an 11th empty zone.
Given that state, my understanding of the second part is that the ZNS SSD
chooses one of the previously 10 zones, and transition the chosen zone to
the Closed state, and then proceeds to write to the new zone which also
implicitly transition it from the Empty state to the Impl. Open state.
After this, there would be 11 active zones in total, 10 in impl. Open
state, and one in closed state.
The above assumes that a ZNS SSD will always transition an implicitly
opened zone to closed state when required to free up resources when
another zone is opened. However, it isn’t strictly said in the ZNS spec.
The paragraph that should cover it is defined in section
2.1.1.4.1 – Managing Resources:
The controller may transition zones in the ZSIO:Implicitly Opened state
to the ZSC:Closed state for resource management purposes.
However, it doesn’t say “when” it should occur. Thus, as the text stand,
it could be misinterpreted that the controller shouldn’t do close a zone
to make room for a new zone. The issue with this, is that it makes the
point of having implicitly managed zones moot.
The ZAC/ZBC specs is more specific and clarifies when a zone should be
closed. I think it would be natural to the same here.
"""
While the Zoned Namespace Command Set Specification hasn't received an
errata yet, it is quite clear that the intention was that an automatic
zone transition was never supposed to be optional, as then the whole
point of having implictly open zones would be pointless. Therefore,
remove the param zoned.auto_transition, as this was never supposed to
be controller implementation specific.
Signed-off-by: Niklas Cassel
---
hw/nvme/ctrl.c | 35 ---
hw/nvme/nvme.h | 1 -
2 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 87aeba0564..b8c8075301 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -34,7 +34,6 @@
* aerl=,aer_max_queued=, \
* mdts=,vsl=, \
* zoned.zasl=, \
- * zoned.auto_transition=, \
* sriov_max_vfs= \
* sriov_vq_flexible= \
* sriov_vi_flexible= \
@@ -106,11 +105,6 @@
* the minimum memory page size (CAP.MPSMIN). The default value is 0 (i.e.
* defaulting to the value of `mdts`).
*
- * - `zoned.auto_transition`
- * Indicates if zones in zone state implicitly opened can be automatically
- * transitioned to zone state closed for resource management purposes.
- * Defaults to 'on'.
- *
* - `sriov_max_vfs`
* Indicates the maximum number of PCIe virtual functions supported
* by the controller. The default value is 0. Specifying a non-zero value
@@ -1867,8 +1861,8 @@ enum {
NVME_ZRM_ZRWA = 1 << 1,
};
-static uint16_t nvme_zrm_open_flags(NvmeCtrl *n, NvmeNamespace *ns,
-NvmeZone *zone, int flags)
+static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
+int flags)
{
int act = 0;
uint16_t status;
@@ -1880,9 +1874,7 @@ static uint16_t nvme_zrm_open_flags(NvmeCtrl *n,
NvmeNamespace *ns,
/* fallthrough */
case NVME_ZONE_STATE_CLOSED:
-if (n->params.auto_transition_zones) {
-nvme_zrm_auto_transition_zone(ns);
-}
+nvme_zrm_auto_transition_zone(ns);
status = nvme_zns_check_resources(ns, act, 1,
(flags & NVME_ZRM_ZRWA) ? 1 : 0);
if (status) {
@@ -1925,10 +1917,9 @@ static uint16_t nvme_zrm_open_flags(NvmeCtrl *n,
NvmeNamespace *ns,
}
}
-static inline uint16_t nvme_zrm_auto(NvmeCtrl *n, NvmeNamespace *ns,
- NvmeZone *zone)
+static inline uint16_t nvme_zrm_auto(NvmeNamespace *ns, NvmeZone *zone)
{
-return nvme_zrm_open_flags(n, ns, zone, NVME_ZRM_AUTO);
+return nvme_zrm_open_flags(ns, zone, NVME_ZRM_AUTO);
}
static void nvme_advance_zone_wp(NvmeNamespace *ns, NvmeZone *zone,
@@ -3065,7 +3056,7 @@ static uint16_t nvme_copy(NvmeCtrl *n, NvmeRequest *req)
goto invalid;
}
-status = nvme_zrm_auto(n, ns, iocb->zone);
+status = nvme_zrm_auto(ns, iocb->zone);
if (status) {
goto invalid;
}
@@ -3471,7 +3462,7 @@ static
