Re: [Qemu-devel] [PATCH v11 3/3] qemu-img info: bitmaps extension new test 239 - Vladimir Sementsov-Ogievskiy

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"qemu-block@nongnu.org" <qemu-block@nongnu.org>
Cc: "eblake@redhat.com" <eblake@redhat.com>,
	"fam@euphon.net" <fam@euphon.net>,
	"armbru@redhat.com" <armbru@redhat.com>,
	"mreitz@redhat.com" <mreitz@redhat.com>,
	"kwolf@redhat.com" <kwolf@redhat.com>,
	"jsnow@redhat.com" <jsnow@redhat.com>,
	Denis Lunev <den@virtuozzo.com>
Subject: Re: [Qemu-devel] [PATCH v11 3/3] qemu-img info: bitmaps extension new test 239
Date: Fri, 1 Feb 2019 15:57:05 +0000	[thread overview]
Message-ID: <252495f5-8ade-336a-658f-9f6e979bda4c@virtuozzo.com> (raw)
In-Reply-To: <1548942405-760115-4-git-send-email-andrey.shinkevich@virtuozzo.com>

31.01.2019 16:46, Andrey Shinkevich wrote:
> A new test file 239 added to the qemu-iotests set. It checks
> the output format of 'qemu-img info' for bitmaps extension of
> qcow2 specific information.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/239     |  74 ++++++++++++++++++++++++++
>   tests/qemu-iotests/239.out | 130 +++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 205 insertions(+)
>   create mode 100755 tests/qemu-iotests/239
>   create mode 100644 tests/qemu-iotests/239.out
> 
> diff --git a/tests/qemu-iotests/239 b/tests/qemu-iotests/239
> new file mode 100755
> index 0000000..bee7943
> --- /dev/null
> +++ b/tests/qemu-iotests/239
> @@ -0,0 +1,74 @@
> +#!/usr/bin/env python
> +#
> +# Test for qcow2 bitmap printed information
> +#
> +# Copyright (c) 2018 Virtuozzo International GmbH
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import iotests
> +import json
> +from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \
> +                    file_path, log
> +
> +iotests.verify_image_format(supported_fmts=['qcow2'])
> +
> +disk = file_path('disk')
> +chunk = 256
> +
> +def print_bitmap():
> +    log('qemu-img info dump:\n')
> +    iotests.img_info_log(disk, extra_args=['--force-share'])
> +    result = json.loads(qemu_img_pipe('info', '--force-share',
> +                                      '--output=json', disk))
> +    if 'bitmaps' in result['format-specific']['data']:
> +        bitmaps = result['format-specific']['data']['bitmaps']
> +        log('The same bitmaps in JSON format:')
> +        log(bitmaps, indent=2)
> +    else:
> +        log('No bitmap in JSON format output')
> +
> +def add_bitmap(bitmap_number, persistent, disabled):
> +    granularity = 2**(13 + bitmap_number)
> +    bitmap_name = 'bitmap-' + str(bitmap_number-1)
> +    vm = iotests.VM().add_drive(disk)
> +    vm.launch()
> +    vm.qmp_log('block-dirty-bitmap-add', node='drive0', name=bitmap_name,
> +               granularity=granularity, persistent=persistent,
> +               disabled=disabled)
> +    vm.shutdown()
> +
> +def write_to_disk(offset, size):
> +    write = 'write {} {}K'.format(offset, size)

I think, you meant {}K {}K. On the other hand, if you want to work with 256K chunks,
I think, better is to define chunk as
chunk = 256 * 1024

> +    qemu_io('-f', iotests.imgfmt, '-c', write, disk)

Hmm, interesting, it's a common preactice to give -f iotests.imgfmt to qemu_io(),
but in reality it's not needed, as qemu_io() automatically set this flag, this
parameter will be passed twice. So, it would be good to fix it in all iotests,
handle '-f' properly in qemu_io(), and I think, in qemu-io utility too, forbid
multiple -f option.

So, here let's use
qemu_io('-c', write, disk)

> +    log('Write ' + str(size) + 'K to disk at offset ' + str(hex(offset)))

hmm, you just do qemu-io output by hand. better log() what qemu_io() returns
with appropriate filters, look in iotest 149

> +
> +
> +qemu_img_create('-f', iotests.imgfmt, disk, '1M')
> +
> +for num in range(1, 4):

good to log('Test {}'.format(num)) here

> +    disabled = False
> +    if num == 2:
> +        disabled = True
> +    add_bitmap(num, bool(num-1), disabled)

more clear: num > 1

> +    write_to_disk((num-1)*chunk, chunk)

Hm, actually this don't change test output, as we don't see dirty bits through
new API. However, it make sense anyway, to be closer to real-life.

> +    print_bitmap()
> +    log('')
> +
> +vm = iotests.VM().add_drive(disk)
> +vm.launch()
> +log('Checking \"in-use\" flag...')

don't back-slash double quotes, it's not needed, as you are inside single quotes.

> +print_bitmap()

I'd prefere to use '--force-share' only for this case, when it's really needed.

> +vm.shutdown()
> diff --git a/tests/qemu-iotests/239.out b/tests/qemu-iotests/239.out

-- 
Best regards,
Vladimir