[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH] block/mirror: check backing in bdr
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH] block/mirror: check backing in bdrv_mirror_top_refresh_filename |
Date: |
Fri, 29 Sep 2017 15:46:29 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
On 09/28/2017 08:03 AM, Vladimir Sementsov-Ogievskiy wrote:
> Backing may be zero after failed bdrv_attach_child in
> bdrv_set_backing_hd, which leads to SIGSEGV.
>
I guess in this case we trust bdrv_set_backing_hd to carry the errp
parameter back to the caller indicating that not only did we fail to set
the new backing_hd, but we've also lost the old one, as evidenced by the
nop-style return in .bdrv_refresh_filename...
so the error case should already be "handled", and this just cleans up a
segfault.
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>
> Hi all.
>
> We have faced into this SIGSEGV because of image locking:
> trying to call qemu-img commit on locked image leads to the following:
>
> Program terminated with signal 11, Segmentation fault.
> #0 bdrv_mirror_top_refresh_filename (bs=0x5616df9a7400,
> opts=0x5616df268400)
> at block/mirror.c:1203
> 1203 bdrv_refresh_filename(bs->backing->bs);
>
> (gdb) bt
> #0 bdrv_mirror_top_refresh_filename (bs=0x5616df9a7400,
> opts=0x5616df268400)
> at block/mirror.c:1203
> #1 0x00005616ddc3d35f in bdrv_refresh_filename (bs=0x5616df9a7400) at
> block.c:4739
> #2 0x00005616ddc3d672 in bdrv_set_backing_hd (address@hidden,
> address@hidden, address@hidden)
> at block.c:2035
> #3 0x00005616ddc3dee3 in bdrv_append (address@hidden,
> address@hidden, address@hidden)
> at block.c:3168
> #4 0x00005616ddc84e5f in mirror_start_job (
> address@hidden "commit", address@hidden,
> address@hidden, address@hidden,
> address@hidden, address@hidden, granularity=65536,
> address@hidden, buf_size=16777216, address@hidden,
> address@hidden,
> address@hidden,
> address@hidden,
> address@hidden, address@hidden <common_block_job_cb>,
> address@hidden, address@hidden,
> address@hidden <commit_active_job_driver>,
> address@hidden, address@hidden,
> address@hidden,
> address@hidden) at block/mirror.c:1314
> #5 0x00005616ddc87580 in commit_active_start (
> address@hidden "commit", address@hidden,
> address@hidden, address@hidden,
> address@hidden, address@hidden,
> address@hidden,
> address@hidden <common_block_job_cb>,
> address@hidden, address@hidden,
> address@hidden) at block/mirror.c:1463
> #6 0x00005616ddc33a68 in img_commit (argc=<optimized out>,
> argv=<optimized out>)
> at qemu-img.c:1013
> #7 0x00005616ddc2fa79 in main (argc=4, argv=0x7ffff7896e00) at
> qemu-img.c:4548
>
>
> (gdb) p bs->backing
> $2 = (BdrvChild *) 0x0
> (gdb) fr 2
> #2 0x00005616ddc3d672 in bdrv_set_backing_hd (address@hidden,
> address@hidden, address@hidden)
> at block.c:2035
> 2035 bdrv_refresh_filename(bs);
> (gdb) p *errp
> $4 = (Error *) 0x5616df1c2660
> (gdb) p **errp
> $5 = {msg = 0x5616df2554e0 "Failed to get \"write\" lock",
> err_class = ERROR_CLASS_GENERIC_ERROR,
> src = 0x5616ddd267fe "block/file-posix.c",
> func = 0x5616ddd26fe0 <__func__.27999> "raw_check_lock_bytes", line =
> 682,
> hint = 0x5616df1fe520}
>
>
> block/mirror.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 6f5cb9f26c..351b80ca2c 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1073,6 +1073,11 @@ static int coroutine_fn
> bdrv_mirror_top_pdiscard(BlockDriverState *bs,
>
> static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict
> *opts)
> {
> + if (bs->backing == NULL) {
> + /* we can be here after failed bdrv_attach_child in
> + * bdrv_set_backing_hd */
> + return;
> + }
> bdrv_refresh_filename(bs->backing->bs);
> pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
> bs->backing->bs->filename);
>
Reviewed-by: John Snow <address@hidden>