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

[Max Reitz <mreitz@redhat.com>]

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 <armbru@redhat.com>
> ---
>   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:

Reviewed-by: Max Reitz <mreitz@redhat.com>