Re: [Qemu-devel] [PATCH v7 12/35] util: let qemu_fd_getlength support block device

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

On 11/03/2015 12:11 AM, Vladimir Sementsov-Ogievskiy wrote:
> On 02.11.2015 12:13, Xiao Guangrong wrote:
>> lseek can not work for all block devices as the man page says:
>> | Some devices are incapable of seeking and POSIX does not specify
>> | which devices must support lseek().
>>
>> This patch tries to add the support on Linux by using BLKGETSIZE64
>> ioctl
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>>   util/osdep.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/util/osdep.c b/util/osdep.c
>> index 5a61e19..b20c793 100644
>> --- a/util/osdep.c
>> +++ b/util/osdep.c
>> @@ -45,6 +45,11 @@
>>   extern int madvise(caddr_t, size_t, int);
>>   #endif
>> +#ifdef CONFIG_LINUX
>> +#include <sys/ioctl.h>
>> +#include <linux/fs.h>
>> +#endif
>> +
>>   #include "qemu-common.h"
>>   #include "qemu/sockets.h"
>>   #include "qemu/error-report.h"
>> @@ -433,6 +438,21 @@ int64_t qemu_fd_getlength(int fd)
>>   {
>>       int64_t size;
>> +#ifdef CONFIG_LINUX
>> +    struct stat stat_buf;
>> +    if (fstat(fd, &stat_buf) < 0) {
>> +        return -errno;
>> +    }
>> +
>> +    if ((S_ISBLK(stat_buf.st_mode)) && !ioctl(fd, BLKGETSIZE64, &size)) {
>> +        /* The size of block device is larger than max int64_t? */
>> +        if (size < 0) {
>> +            return -EOVERFLOW;
>> +        }
>> +        return size;
>> +    }
>> +#endif
>> +
>>       size = lseek(fd, 0, SEEK_END);
>>       if (size < 0) {
>>           return -errno;
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> just a question: is there any use for stat.st_size ? Is it always worse then lseek?

The man page says:
The  st_size field gives the size of the file (if it is a regular file or a symbolic link)
in bytes.  The size of a symbolic link is the length of the pathname it contains, without a
terminating null byte.

So it can not work on symbolic link.

> Does it work for
> blk?
>

Quickly checked with a program written by myself and 'stat' command, the answer is NO. :)

> also, "This patch tries to add..". Hmm. It looks like this patch is not sure about will it success.
> I'd prefer "This patch adds", but this is not important
>

Thanks for your sharing. I did not know the different, now, i got it. :)