[libvirt] [PATCH 1/1] storage: Fix a vol-clone bug for ppc64

[Li Zhang]

On 2013年11月08日 15:10, Ján Tomko wrote:
> On 11/08/2013 06:25 AM, Li Zhang wrote:
>> On 2013年11月07日 18:41, Ján Tomko wrote:
>>> On 11/07/2013 09:35 AM, Li Zhang wrote:
>>>> Currently, wbytes is defined as size_t type which is 8 bytes on ppc64,
>>>> but args's value in ioctl(fd, args..) in kernel is unsigned int.
>>>> So it will read more 4 bytes by ioctl() with size_t from memory which
>>>> gets a large number and causes out of memory error.
>>> On x86_64 size_t is 8 bytes and int is 4 bytes, I guess this is an
>>> endianness issue.
>> It doesn't look like the endianness issue.
>> It appends more 32 0s to the value.
>> I need to trace kernel to see how this happens.
>>
>> Here is what I get on PPC64.
>> blksz = 0x1000000000000, pbsz = 0x20000000000
>>
>> Actually, they should be :
>> blksz = 0x10000, pbsz = 0x200
>>
>> On x86_64, they can get right value.
>>
> That looks like endianness to me :)
> 0x200 on little endian:
> 4-byte int:    0x00 0x02 0x00 0x00
> 8-byte size_t: 0x00 0x02 0x00 0x00  0x00 0x00 0x00 0x00
>
> 0x200 on big endian:
> 4-byte int:    0x00 0x00 0x02 0x00
> 8-byte size_t: 0x00 0x00 0x00 0x00  0x00 0x00 0x02 0x00
> 8-byte size_t with the int written in the first four bytes:
>                 0x00 0x00 0x02 0x00  0x00 0x00 0x00 0x00
> Which is what you get in pbsz on PPC64.

Ah,  I converted the big endian incorrectly.

Thanks for your explanation.

>
>>>> This patch changes size_t to unsigned int to synchronize with kernel.
>>> The type in the kernel seems to be int:
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/block/ioctl.c#n363
>>>
>>> https://lkml.org/lkml/2013/11/1/620
>> Ah, you are right, should be int. :)
>>
>>>> Signed-off-by: Li Zhang <zhlcindy at linux.vnet.ibm.com>
>>>> ---
>>>>    src/storage/storage_backend.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
>>>> index 4b1b00d..f510080 100644
>>>> --- a/src/storage/storage_backend.c
>>>> +++ b/src/storage/storage_backend.c
>>>> @@ -135,7 +135,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
>>>>        int amtread = -1;
>>>>        int ret = 0;
>>>>        size_t rbytes = READ_BLOCK_SIZE_DEFAULT;
>>>> -    size_t wbytes = 0;
>>>> +    unsigned int wbytes = 0;
>>>>        int interval;
>>>>        char *zerobuf = NULL;
>>>>        char *buf = NULL;
>>>>
>>> I'll push this with s/unsigned // later, unless someone has a different
>>> opinion.
>> Got it, thanks.
>>
> Now pushed.
>
> Jan
>
>