Am 23.12.2019 um 14:40 hat Eiichi Tsukata geschrieben:
>
>
> On 2019/12/23 21:40, Vladimir Sementsov-Ogievskiy wrote:
> > 23.12.2019 12:06, Eiichi Tsukata wrote:
> >> bdrv_open_driver() allocates bs->opaque according to drv->instance_size.
> >> There is no need to allocate it and overwrite opaque in
> >> bdrv_backup_top_append().
> >>
> >> Reproducer:
> >>
> >> $ QTEST_QEMU_BINARY=./x86_64-softmmu/qemu-system-x86_64 valgrind -q
> >> --leak-check=full tests/test-replication -p /replication/secondary/start
> >> ==29792== 24 bytes in 1 blocks are definitely lost in loss record 52 of
> >> 226
> >> ==29792== at 0x483AB1A: calloc (vg_replace_malloc.c:762)
> >> ==29792== by 0x4B07CE0: g_malloc0 (in
> >> /usr/lib64/libglib-2.0.so.0.6000.7)
> >> ==29792== by 0x12BAB9: bdrv_open_driver (block.c:1289)
> >> ==29792== by 0x12BEA9: bdrv_new_open_driver (block.c:1359)
> >> ==29792== by 0x1D15CB: bdrv_backup_top_append (backup-top.c:190)
> >> ==29792== by 0x1CC11A: backup_job_create (backup.c:439)
> >> ==29792== by 0x1CD542: replication_start (replication.c:544)
> >> ==29792== by 0x1401B9: replication_start_all (replication.c:52)
> >> ==29792== by 0x128B50: test_secondary_start (test-replication.c:427)
> >> ...
> >>
> >> Fixes: 7df7868b9640 ("block: introduce backup-top filter driver")
> >> Signed-off-by: Eiichi Tsukata <de...@etsukata.com>
> >> ---
> >> block/backup-top.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/block/backup-top.c b/block/backup-top.c
> >> index 7cdb1f8eba..617217374d 100644
> >> --- a/block/backup-top.c
> >> +++ b/block/backup-top.c
> >> @@ -196,7 +196,7 @@ BlockDriverState
> >> *bdrv_backup_top_append(BlockDriverState *source,
> >> }
> >>
> >> top->total_sectors = source->total_sectors;
> >> - top->opaque = state = g_new0(BDRVBackupTopState, 1);
> >> + state = top->opaque;
> >>
> >> bdrv_ref(target);
> >> state->target = bdrv_attach_child(top, target, "target",
> >> &child_file, errp);
> >>
> >
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> >
> > Hmm, it was not my idea, I just copied it from mirror.. And there should be
> > the same leak. and
> > may be in other places:
> >
> > # git grep 'opaque =.*g_new'
> > block/backup-top.c: top->opaque = state = g_new0(BDRVBackupTopState, 1);
Fixed by this patch.
> > block/file-posix.c: state->opaque = g_new0(BDRVRawReopenState, 1);
> > block/gluster.c: state->opaque = g_new0(BDRVGlusterReopenState, 1);
> > block/raw-format.c: reopen_state->opaque = g_new0(BDRVRawState, 1);
> > block/sheepdog.c: re_s = state->opaque = g_new0(BDRVSheepdogReopenState,
> > 1);
Doing this for reopen state is fine.
> > block/iscsi.c: bs->opaque = g_new0(struct IscsiLun, 1);
This one looks kind of questionable. It basically builds its
BlockDriveState manually without using any of the block layer open
functions.
> > block/mirror.c: bs_opaque = g_new0(MirrorBDSOpaque, 1);
Harmless as Eiichi explained below, but not nice either.
> Thanks for reviewing.
> As you say, block/mirror.c has similar code. But it does not cause the leak.
> The difference is bdrv_mirror_top BlockDriver does not have .instance_size
> whereas bdrv_backup_top_filter BlockDriver has .instance_size =
> sizeof(BDRVBackupTopState).
> So when bdrv_open_driver() is called from mirror.c, g_malloc0(0) is
> called allocating nothing.
I think it should still be changed just because it would make the code
cleaner. It's always better to use common infrastructure than
reimplementing it locally.
Kevin
