Re: [Qemu-devel] [PATCH v3 12/23] virtio-blk: Drop redundant VirtIOBlock member conf

[Markus Armbruster]

Max Reitz <address@hidden> writes:

> On 16.09.2014 20:12, Markus Armbruster wrote:
>> Commit 12c5674 turned it into a pointer to member blk.conf.
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>   hw/block/virtio-blk.c          | 28 ++++++++++++++--------------
>>   include/hw/virtio/virtio-blk.h |  1 -
>>   2 files changed, 14 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index 38ad38f..5943af5 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -298,7 +298,7 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
>>       if (sector & dev->sector_mask) {
>>           return false;
>>       }
>> -    if (size % dev->conf->logical_block_size) {
>> +    if (size % dev->blk.conf.logical_block_size) {
>>           return false;
>>       }
>>       bdrv_get_geometry(dev->bs, &total_sectors);
>> @@ -519,19 +519,20 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>>   static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>>   {
>>       VirtIOBlock *s = VIRTIO_BLK(vdev);
>> +    BlockConf *conf = &s->blk.conf;
>>       struct virtio_blk_config blkcfg;
>>       uint64_t capacity;
>> -    int blk_size = s->conf->logical_block_size;
>> +    int blk_size = conf->logical_block_size;
>>         bdrv_get_geometry(s->bs, &capacity);
>>       memset(&blkcfg, 0, sizeof(blkcfg));
>>       virtio_stq_p(vdev, &blkcfg.capacity, capacity);
>>       virtio_stl_p(vdev, &blkcfg.seg_max, 128 - 2);
>> -    virtio_stw_p(vdev, &blkcfg.cylinders, s->conf->cyls);
>> +    virtio_stw_p(vdev, &blkcfg.cylinders, conf->cyls);
>>       virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
>> -    virtio_stw_p(vdev, &blkcfg.min_io_size, s->conf->min_io_size / 
>> blk_size);
>> -    virtio_stw_p(vdev, &blkcfg.opt_io_size, s->conf->opt_io_size / 
>> blk_size);
>> -    blkcfg.heads = s->conf->heads;
>> +    virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
>> +    virtio_stw_p(vdev, &blkcfg.opt_io_size, conf->opt_io_size / blk_size);
>> +    blkcfg.heads = conf->heads;
>>       /*
>>        * We must ensure that the block device capacity is a multiple of
>>        * the logical block size. If that is not the case, let's use
>> @@ -543,13 +544,13 @@ static void virtio_blk_update_config(VirtIODevice 
>> *vdev, uint8_t *config)
>>        * divided by 512 - instead it is the amount of blk_size blocks
>>        * per track (cylinder).
>>        */
>> -    if (bdrv_getlength(s->bs) /  s->conf->heads / s->conf->secs % blk_size) 
>> {
>> -        blkcfg.sectors = s->conf->secs & ~s->sector_mask;
>> +    if (bdrv_getlength(s->bs) /  conf->heads / conf->secs % blk_size) {
>> +        blkcfg.sectors = conf->secs & ~s->sector_mask;
>>       } else {
>> -        blkcfg.sectors = s->conf->secs;
>> +        blkcfg.sectors = conf->secs;
>>       }
>>       blkcfg.size_max = 0;
>> -    blkcfg.physical_block_exp = get_physical_block_exp(s->conf);
>> +    blkcfg.physical_block_exp = get_physical_block_exp(&s->blk.conf);
>
> Is there a reason for you not using "conf" instead of "&s->blk.conf" here?
>
> Of course, it's not wrong, so with the one or the other:

Looks like an editing accident to me.  I'll clean it up in v4.

> Reviewed-by: Max Reitz <address@hidden>

Thanks!