Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate

[Eric Blake <eblake@redhat.com>]

On 02/28/2018 07:53 AM, Daniel P. Berrangé wrote:

>>>> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>>>>   
>>>>           buf = g_malloc0(65536);
>>>>   
>>>> -        result = lseek(fd, current_length, SEEK_SET);
>>>> -        if (result < 0) {
>>>> +        seek_result = lseek(fd, current_length, SEEK_SET);
>>>> +        if (seek_result < 0) {
>>>
>>> off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
>>> exact value (off_t)-1 indicates an error. So this needs to be
>>>
>>>     if (seek_result == (off_t)-1) {
>>>        ...
>>>     }

No, off_t is ALWAYS signed, even when it is 32-bit.  The cast helps code 
that is written for 64-bit off_t to still work when compiled with the 
small file model where a 32-bit value is used (but we don't have to 
worry about that, as we always compile for large file mode with 64-bit 
off_t).

>>
>> Hmmm... On my system, it appears to be a long int[1].  And
>> find_allocation() does an off_t < 0 comparison already.  And
>> "man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
>> types."
> 
> Hmm, that's odd then - lseek man page explicitly said it must be cast,
> which suggested to me it could be unsigned:
> 
>     RETURN VALUE
>         Upon successful completion, lseek() returns the resulting offset  loca‐
>         tion  as  measured  in bytes from the beginning of the file.  On error,
>         the value (off_t) -1 is returned and  errno  is  set  to  indicate  the
>         error.
> 
> CC'ing Eric for the "official" POSIX answer....

It MAY be that the man page mentions a cast because '-1' is an int but 
'(off_t) -1' can be larger than an int.  But it may also be that you've 
uncovered something worth reporting as a bug to the man page project.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org