Re: [Qemu-devel] [PATCH 3/5] Write table offset and size in one syscall.

[Gleb Natapov]

On Mon, Nov 24, 2008 at 10:42:24AM -0600, Anthony Liguori wrote:
>>       new_l1_size = s->l1_size;
>>      if (min_size <= new_l1_size)
>> @@ -460,14 +459,12 @@ static int grow_l1_table(BlockDriverState *bs, int 
>> min_size)
>>          new_l1_table[i] = be64_to_cpu(new_l1_table[i]);
>>       /* set new table */
>> -    data64 = cpu_to_be64(new_l1_table_offset);
>> -    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_table_offset),
>> -                    &data64, sizeof(data64)) != sizeof(data64))
>> -        goto fail;
>> -    data32 = cpu_to_be32(new_l1_size);
>> -    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size),
>> -                    &data32, sizeof(data32)) != sizeof(data32))
>> +    *(uint32_t*)data = cpu_to_be32(new_l1_size);
>> +    *(uint64_t*)&data[4] = cpu_to_be64(new_l1_table_offset);
>> +    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size), data,
>> +                sizeof(data)) != sizeof(data))
>>          goto fail;
>>
>>   
>
> Why not just introduces a uint8_t data[12] in this function, memcpy to  
> the right offsets, and do one brdv_pwrite?  Then you don't need to do  
> weird things with packing.
>
It is exactly what I did except memcpy part. What can go wrong with
packing the way I did it?

>>          be64_to_cpus(&new_table[i]);
>>  -    data64 = cpu_to_be64(table_offset);
>> +    *(uint64_t*)data = cpu_to_be64(table_offset);
>> +    *(uint32_t*)&data[8] = cpu_to_be32(refcount_table_clusters);
>>      if (bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_offset),
>> -                    &data64, sizeof(data64)) != sizeof(data64))
>> -        goto fail;
>> -    data32 = cpu_to_be32(refcount_table_clusters);
>> -    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_clusters),
>> -                    &data32, sizeof(data32)) != sizeof(data32))
>> +                    data, sizeof(data)) != sizeof(data))
>>          goto fail;
>>   
>
> Same here.  Alternatively, you could use the cpu_to_beXXs() variants and  
> just pass in pointer offsets.
>
That is good idea. I'll use cpu_to_beXXs().

--
                        Gleb.