On Fri, Apr 11, 2025 at 10:54:57PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 11.04.25 04:04, Eric Blake wrote:
> > When doing a sync=full mirroring, QMP drive-mirror requests full
> > zeroing if it did not just create the destination, and blockdev-mirror
> > requests full zeroing unconditionally. This is because during a full
> > sync, we must ensure that the portions of the disk that are not
> > otherwise touched by the source still read as zero upon completion.
...
> >
> > +++ b/block/mirror.c
> > @@ -841,14 +841,20 @@ static int coroutine_fn GRAPH_UNLOCKED
> > mirror_dirty_init(MirrorBlockJob *s)
> > int64_t offset;
> > BlockDriverState *bs;
> > BlockDriverState *target_bs = blk_bs(s->target);
> > - int ret = -1;
> > + int ret;
>
> I think, it was to avoid Coverity false-positive around further code
>
> WITH_GRAPH_RDLOCK_GUARD() {
> ret = bdrv_co_is_allocated_above(bs, s->base_overlay, true,
> offset,
> bytes, &count);
> }
> if (ret < 0) {
> return ret;
> }
>
> which you don't touch here. I think "= -1;" should be kept. Or I missed
> static analyzes revolution (if so, it should be mentioned in commit message).
Fair enough, for keeping something to avoid Coverity false positives.
HOWEVER, initializing to -1 is wrong, since, as you just showed,
elsewhere in the function we return -errno on failure, and -1 is not
always the right error. If I'm going to keep an initializer, it will
be -EIO rather than -1. Furthermore, if I understand
WITH_GRAPH_RDLOCK_GUARD() correctly, we could hoist the 'if (ret < 0)'
check inside the guarded region and still get the same behavior, but
with a different visibility to the analyzer.
>
> > int64_t count;
> >
> > bdrv_graph_co_rdlock();
> > bs = s->mirror_top_bs->backing->bs;
> > + if (s->zero_target) {
> > + ret = bdrv_co_is_zero_fast(target_bs, 0, s->bdev_length);
> > + }
> > bdrv_graph_co_rdunlock();
The rdlock is important, but rewriting it into the
WITH_GRAPH_RDLOCK_GUARD() scope does make it look nicer. Adjusting
that for v2.
> >
> > - if (s->zero_target) {
> > + if (s->zero_target && ret <= 0) {
> > + if (ret < 0) {
> > + return ret;
> > + }
> > if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
> > bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length);
> > return 0;
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
