Re: [Qemu-devel] [PATCH v2 2/5] block: Guarantee that *file is set on bdrv_get_block_status()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]

On 2017-05-24 22:28, Eric Blake wrote:
> We document that *file is valid if the return is not an error and
> includes BDRV_BLOCK_OFFSET_VALID, but forgot to obey this contract
> when a driver (such as blkdebug) lacks a callback.  Broken in
> commit 67a0fd2 (v2.6), when we added the file parameter.

Not sure if I'd call that "broken", as in the part participle of "break"
because there was never any breaking. It just didn't abide by the
contract from the start.

> Enhance qemu-iotest 177 to cover this, using a sequence that would
> print garbage or even SEGV, because it was dererefencing through
> uninitialized memory.  [The resulting test output shows that we
> have less-than-ideal block status from the blkdebug driver, but
> that's a separate fix coming up soon.]
> 
> Setting *file on all paths that return BDRV_BLOCK_OFFSET_VALID is
> enough to fix the crash, but we can go one step further: always
> setting *file, even on error, means that a broken caller that
> blindly dereferences file without checking for error is now more
> likely to get a reliable SEGV instead of randomly acting on garbage,
> making it easier to diagnose such buggy callers.  Adding an
> assertion that file is set where expected doesn't hurt either.
> 
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: drop redundant assignment
> ---
>  block/io.c                 | 5 +++--
>  tests/qemu-iotests/177     | 3 +++
>  tests/qemu-iotests/177.out | 2 ++
>  3 files changed, 8 insertions(+), 2 deletions(-)

Anyway,

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]