Re: [Qemu-devel] [PATCH v3 1/3] raw-posix: Fix raw_co_get_block_status() after EOF

[Max Reitz <mreitz@redhat.com>]

On 2014-10-23 at 09:27, Max Reitz wrote:
> On 2014-10-22 at 18:57, Eric Blake wrote:
>> On 10/22/2014 09:57 AM, Max Reitz wrote:
>>> As its comment states, raw_co_get_block_status() should unconditionally
>>> return 0 and set *pnum to 0 for after EOF.
>>>
>>> An assertion after lseek(..., SEEK_HOLE) tried to catch this case by
>>> asserting that errno != -ENXIO (which would indicate a position after
>>> the EOF); but it should be errno != ENXIO instead. Regardless of that,
>>> there should be no such assertion at all. If bdrv_getlength() returned
>>> an outdated value and the image has been resized outside of qemu,
>>> lseek() will return with errno == ENXIO. Just return that value as an
>>> error then.
>>>
>>> Setting *pnum to 0 and returning 0 should not be done here, as in that
>>> case we should update the device length as well. So, from qemu's
>>> perspective, the file has not been resized; it's just that there was an
>>> error querying sectors beyond a certain point (the actual file size).
>>>
>>> Additionally, nb_sectors should be clamped against the image end. This
>>> was probably not an issue if FIEMAP or SEEK_HOLE/SEEK_DATA worked, but
>>> the fallback did not take this case into account.
>>>
>>> Reported-by: Kevin Wolf <kwolf@redhat.com>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>   block/raw-posix.c | 14 ++++++++++----
>>>   1 file changed, 10 insertions(+), 4 deletions(-)
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>>> +    if (total_size < 0) {
>>> +        return total_size;
>>> +    } else if (start >= total_size) {
>>> +        *pnum = 0;
>>> +        return 0;
>>> +    } else if (start + nb_sectors * BDRV_SECTOR_SIZE > total_size) {
>>> +        nb_sectors = (total_size - start) / BDRV_SECTOR_SIZE;
>> Should this round up instead of truncate?  But it would only matter for
>> a file size that is not a multiple of sectors, where we probably have
>> other issues, and where reporting just the full sectors also seems
>> reasonable.
>
> There already was a series (as far as I remember) that somehow tried 
> to make all or at least some block drivers compatible with sizes which 
> are not a multiple of the sector size, so I shouldn't be nullifying 
> that work. Will use ROUND_UP().

Sorry, DIV_ROUND_UP(), of course.

Max