Re: [PULL 6/6] iotests: add backup-discard-source

2024-04-30 Thread Vladimir Sementsov-Ogievskiy 

On 30.04.24 12:13, Kevin Wolf wrote:

Am 29.04.2024 um 20:39 hat Vladimir Sementsov-Ogievskiy geschrieben:

[Add John]

On 29.04.24 17:18, Richard Henderson wrote:

On 4/29/24 04:51, Vladimir Sementsov-Ogievskiy wrote:

Add test for a new backup option: discard-source.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Fiona Ebner 
Tested-by: Fiona Ebner 
Message-Id: <20240313152822.626493-6-vsement...@yandex-team.ru>
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
   .../qemu-iotests/tests/backup-discard-source  | 152 ++
   .../tests/backup-discard-source.out   |   5 +
   2 files changed, 157 insertions(+)
   create mode 100755 tests/qemu-iotests/tests/backup-discard-source
   create mode 100644 tests/qemu-iotests/tests/backup-discard-source.out


This fails check-python-minreqs

    https://gitlab.com/qemu-project/qemu/-/jobs/6739551782

It appears to be a pylint issue.




tests/export-incoming-iothread:1:0: R0801: Similar lines in 2 files
==tests.backup-discard-source:[52:61]
==tests.copy-before-write:[54:63]
 'file': {
 'driver': iotests.imgfmt,
 'file': {
 'driver': 'file',
 'filename': source_img,
 }
 },
 'target': {
 'driver': iotests.imgfmt, (duplicate-code)



Hmm. I don't think, that something should be changed in code.
splitting out part of this json object to a function? That's a test
for QMP command, and it's good that we see the command as is in one
place. I can swap some lines or rename variables to hack the linter,
but I'd prefer not doing so:)


For me that looks like a false-positive: yes it's a duplication, but
it should better be duplication, than complicating raw json objects by
reusing common parts.


What to do? As described in 22305c2a081b8b6 "python: Reduce strictness
of pylint's duplicate-code check", this check could be either be
disabled completely, or we can increase min-similarity-lines config
value.

I'd just disable it completely. Any thoughts?


I think it would make sense to treat tests differently from real
production code. In tests/, some duplication is bound to happen and
trying to unify things across test cases (which would mean moving to
iotests.py) often doesn't make sense. On the other hand, in python/ we
would probably want to unify duplicated code.



Agree. Happily, it turns out that tests already have separate tests/qemu-iotests/pylintrc 
file, so that's not a problem. Still I decided to anot disable the check completely, but 
just bump the limit, see "[PATCH] iotests/pylintrc: allow up to 10 similar 
lines"

--
Best regards,
Vladimir

Re: [PULL 6/6] iotests: add backup-discard-source

2024-04-30 Thread Kevin Wolf 
Am 29.04.2024 um 20:39 hat Vladimir Sementsov-Ogievskiy geschrieben:
> [Add John]
> 
> On 29.04.24 17:18, Richard Henderson wrote:
> > On 4/29/24 04:51, Vladimir Sementsov-Ogievskiy wrote:
> > > Add test for a new backup option: discard-source.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy 
> > > Reviewed-by: Fiona Ebner 
> > > Tested-by: Fiona Ebner 
> > > Message-Id: <20240313152822.626493-6-vsement...@yandex-team.ru>
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy 
> > > ---
> > >   .../qemu-iotests/tests/backup-discard-source  | 152 ++
> > >   .../tests/backup-discard-source.out   |   5 +
> > >   2 files changed, 157 insertions(+)
> > >   create mode 100755 tests/qemu-iotests/tests/backup-discard-source
> > >   create mode 100644 tests/qemu-iotests/tests/backup-discard-source.out
> > 
> > This fails check-python-minreqs
> > 
> >    https://gitlab.com/qemu-project/qemu/-/jobs/6739551782
> > 
> > It appears to be a pylint issue.
> > 
> > 
> 
> tests/export-incoming-iothread:1:0: R0801: Similar lines in 2 files
> ==tests.backup-discard-source:[52:61]
> ==tests.copy-before-write:[54:63]
> 'file': {
> 'driver': iotests.imgfmt,
> 'file': {
> 'driver': 'file',
> 'filename': source_img,
> }
> },
> 'target': {
> 'driver': iotests.imgfmt, (duplicate-code)
> 
> 
> 
> Hmm. I don't think, that something should be changed in code.
> splitting out part of this json object to a function? That's a test
> for QMP command, and it's good that we see the command as is in one
> place. I can swap some lines or rename variables to hack the linter,
> but I'd prefer not doing so:)
> 
> 
> For me that looks like a false-positive: yes it's a duplication, but
> it should better be duplication, than complicating raw json objects by
> reusing common parts.
> 
> 
> What to do? As described in 22305c2a081b8b6 "python: Reduce strictness
> of pylint's duplicate-code check", this check could be either be
> disabled completely, or we can increase min-similarity-lines config
> value.
> 
> I'd just disable it completely. Any thoughts?

I think it would make sense to treat tests differently from real
production code. In tests/, some duplication is bound to happen and
trying to unify things across test cases (which would mean moving to
iotests.py) often doesn't make sense. On the other hand, in python/ we
would probably want to unify duplicated code.

Kevin

Re: [PULL 6/6] iotests: add backup-discard-source

2024-04-29 Thread Vladimir Sementsov-Ogievskiy 

[Add John]

On 29.04.24 17:18, Richard Henderson wrote:

On 4/29/24 04:51, Vladimir Sementsov-Ogievskiy wrote:

Add test for a new backup option: discard-source.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Fiona Ebner 
Tested-by: Fiona Ebner 
Message-Id: <20240313152822.626493-6-vsement...@yandex-team.ru>
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  .../qemu-iotests/tests/backup-discard-source  | 152 ++
  .../tests/backup-discard-source.out   |   5 +
  2 files changed, 157 insertions(+)
  create mode 100755 tests/qemu-iotests/tests/backup-discard-source
  create mode 100644 tests/qemu-iotests/tests/backup-discard-source.out


This fails check-python-minreqs

   https://gitlab.com/qemu-project/qemu/-/jobs/6739551782

It appears to be a pylint issue.




tests/export-incoming-iothread:1:0: R0801: Similar lines in 2 files
==tests.backup-discard-source:[52:61]
==tests.copy-before-write:[54:63]
'file': {
'driver': iotests.imgfmt,
'file': {
'driver': 'file',
'filename': source_img,
}
},
'target': {
'driver': iotests.imgfmt, (duplicate-code)



Hmm. I don't think, that something should be changed in code. splitting out 
part of this json object to a function? That's a test for QMP command, and it's 
good that we see the command as is in one place. I can swap some lines or 
rename variables to hack the linter, but I'd prefer not doing so:)


For me that looks like a false-positive: yes it's a duplication, but it should 
better be duplication, than complicating raw json objects by reusing common 
parts.


What to do? As described in 22305c2a081b8b6 "python: Reduce strictness of pylint's 
duplicate-code check", this check could be either be disabled completely, or we can 
increase min-similarity-lines config value.

I'd just disable it completely. Any thoughts?


--
Best regards,
Vladimir

Re: [PULL 6/6] iotests: add backup-discard-source

2024-04-29 Thread Richard Henderson 

On 4/29/24 04:51, Vladimir Sementsov-Ogievskiy wrote:

Add test for a new backup option: discard-source.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Fiona Ebner 
Tested-by: Fiona Ebner 
Message-Id: <20240313152822.626493-6-vsement...@yandex-team.ru>
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  .../qemu-iotests/tests/backup-discard-source  | 152 ++
  .../tests/backup-discard-source.out   |   5 +
  2 files changed, 157 insertions(+)
  create mode 100755 tests/qemu-iotests/tests/backup-discard-source
  create mode 100644 tests/qemu-iotests/tests/backup-discard-source.out


This fails check-python-minreqs

  https://gitlab.com/qemu-project/qemu/-/jobs/6739551782

It appears to be a pylint issue.


r~

[PULL 6/6] iotests: add backup-discard-source

2024-04-29 Thread Vladimir Sementsov-Ogievskiy 
Add test for a new backup option: discard-source.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Fiona Ebner 
Tested-by: Fiona Ebner 
Message-Id: <20240313152822.626493-6-vsement...@yandex-team.ru>
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 .../qemu-iotests/tests/backup-discard-source  | 152 ++
 .../tests/backup-discard-source.out   |   5 +
 2 files changed, 157 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/backup-discard-source
 create mode 100644 tests/qemu-iotests/tests/backup-discard-source.out

diff --git a/tests/qemu-iotests/tests/backup-discard-source 
b/tests/qemu-iotests/tests/backup-discard-source
new file mode 100755
index 00..2391b12acd
--- /dev/null
+++ b/tests/qemu-iotests/tests/backup-discard-source
@@ -0,0 +1,152 @@
+#!/usr/bin/env python3
+#
+# Test backup discard-source parameter
+#
+# Copyright (c) Virtuozzo International GmbH.
+# Copyright (c) Yandex
+#
+# 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 .
+#
+
+import os
+
+import iotests
+from iotests import qemu_img_create, qemu_img_map, qemu_io
+
+
+temp_img = os.path.join(iotests.test_dir, 'temp')
+source_img = os.path.join(iotests.test_dir, 'source')
+target_img = os.path.join(iotests.test_dir, 'target')
+size = '1M'
+
+
+def get_actual_size(vm, node_name):
+nodes = vm.cmd('query-named-block-nodes', flat=True)
+node = next(n for n in nodes if n['node-name'] == node_name)
+return node['image']['actual-size']
+
+
+class TestBackup(iotests.QMPTestCase):
+def setUp(self):
+qemu_img_create('-f', iotests.imgfmt, source_img, size)
+qemu_img_create('-f', iotests.imgfmt, temp_img, size)
+qemu_img_create('-f', iotests.imgfmt, target_img, size)
+qemu_io('-c', 'write 0 1M', source_img)
+
+self.vm = iotests.VM()
+self.vm.launch()
+
+self.vm.cmd('blockdev-add', {
+'node-name': 'cbw',
+'driver': 'copy-before-write',
+'file': {
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': source_img,
+}
+},
+'target': {
+'driver': iotests.imgfmt,
+'discard': 'unmap',
+'node-name': 'temp',
+'file': {
+'driver': 'file',
+'filename': temp_img
+}
+}
+})
+
+self.vm.cmd('blockdev-add', {
+'node-name': 'access',
+'discard': 'unmap',
+'driver': 'snapshot-access',
+'file': 'cbw'
+})
+
+self.vm.cmd('blockdev-add', {
+'driver': iotests.imgfmt,
+'node-name': 'target',
+'file': {
+'driver': 'file',
+'filename': target_img
+}
+})
+
+self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024)
+
+def tearDown(self):
+# That should fail, because region is discarded
+self.vm.hmp_qemu_io('access', 'read 0 1M')
+
+self.vm.shutdown()
+
+self.assertTrue('read failed: Permission denied' in self.vm.get_log())
+
+# Final check that temp image is empty
+mapping = qemu_img_map(temp_img)
+self.assertEqual(len(mapping), 1)
+self.assertEqual(mapping[0]['start'], 0)
+self.assertEqual(mapping[0]['length'], 1024 * 1024)
+self.assertEqual(mapping[0]['data'], False)
+
+os.remove(temp_img)
+os.remove(source_img)
+os.remove(target_img)
+
+def do_backup(self):
+self.vm.cmd('blockdev-backup', device='access',
+sync='full', target='target',
+job_id='backup0',
+discard_source=True)
+
+self.vm.event_wait(name='BLOCK_JOB_COMPLETED')
+
+def test_discard_written(self):
+"""
+1. Guest writes
+2. copy-before-write operation, data is stored to temp
+3. start backup(discard_source=True), check that data is
+   removed from temp
+"""
+# Trigger copy-before-write operation
+result = self.vm.hmp_qemu_io('cbw', 'write 0 1M')
+self.assert_qmp(result, 'return', '')
+
+# Check that data is written to temporary image
+