Change in vdsm[master]: qemuimg: add backing file to convert

nsoffer at redhat.com nsoffer at redhat.com
Thu Sep 25 16:10:13 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: qemuimg: add backing file to convert
......................................................................


Patch Set 1:

(11 comments)

http://gerrit.ovirt.org/#/c/33354/1//COMMIT_MSG
Commit Message:

Line 6: 
Line 7: qemuimg: add backing file to convert
Line 8: 
Line 9: This patch adds the support for the backing file in the convert qemu-img
Line 10: command.
Please explain why we need this support in the commit message.
Line 11: 
Line 12: Change-Id: I2d6f254f5750fa249809469ecc764a73c91914d4


http://gerrit.ovirt.org/#/c/33354/1/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 182: 
Line 183:     return check
Line 184: 
Line 185: 
Line 186: def _qcow2_backing_opts(backing, backingFormat):
This method does not add enough value and only makes understanding the code harder.

What can be more useful is a generic function receiving list of option tuples, and returning a string "k1=v1,k2=v2...".

Then we can use it like this:

    options = []
    if foo:
        options.append(('foo', foo))
        if bar:
            options.append(('bar', bar))
    if options:
        cmd.extend(('-o', _format_options(options))
Line 187:     options = []
Line 188: 
Line 189:     options.append('backing_file=' + str(backing))
Line 190:     if backingFormat:


Line 185: 
Line 186: def _qcow2_backing_opts(backing, backingFormat):
Line 187:     options = []
Line 188: 
Line 189:     options.append('backing_file=' + str(backing))
Why convert it to string? if you send invalid input this should blow, not guess.

This would create this:

    >>> 'key=' + str(())
    'key=()'

Instead of:

    >>> 'key=' + ()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: cannot concatenate 'str' and 'tuple' objects
Line 190:     if backingFormat:
Line 191:         options.append('backing_fmt=' + str(backingFormat))
Line 192: 
Line 193:     return options


Line 187:     options = []
Line 188: 
Line 189:     options.append('backing_file=' + str(backing))
Line 190:     if backingFormat:
Line 191:         options.append('backing_fmt=' + str(backingFormat))
Same - blow if a number of some other bad value, used, don't try to make it legal value.
Line 192: 
Line 193:     return options
Line 194: 
Line 195: 


Line 193:     return options
Line 194: 
Line 195: 
Line 196: def convert(srcImage, dstImage, stop, srcFormat=None, dstFormat=None,
Line 197:             backing=None, backingFormat=None):
I think backing should be backingFile - same term used by qemuimg. Unless we used the term backing everywhere (I did not check).
Line 198:     cmd = [_qemuimg.cmd, "convert", "-t", "none"]
Line 199:     options = []
Line 200:     cwdPath = None
Line 201: 


Line 195: 
Line 196: def convert(srcImage, dstImage, stop, srcFormat=None, dstFormat=None,
Line 197:             backing=None, backingFormat=None):
Line 198:     cmd = [_qemuimg.cmd, "convert", "-t", "none"]
Line 199:     options = []
options is relevant only when dstFormat is specified, so it should be limited to that block scope.
Line 200:     cwdPath = None
Line 201: 
Line 202:     if srcFormat:
Line 203:         cmd.extend(("-f", srcFormat))


Line 218:                 options.extend(
Line 219:                     _qcow2_backing_opts(backing, backingFormat))
Line 220: 
Line 221:     if options:
Line 222:         cmd.extend(('-o', ','.join(options)))
Should move into the block.
Line 223: 
Line 224:     cmd.append(dstImage)
Line 225: 
Line 226:     (rc, out, err) = utils.watchCmd(


http://gerrit.ovirt.org/#/c/33354/1/tests/qemuimgTests.py
File tests/qemuimgTests.py:

Line 184:         with FakeCmd(utils, 'execCmd', qcow2_compat_unsupported):
Line 185:             with FakeCmd(utils, 'watchCmd', convert):
Line 186:                 qemuimg.convert('src', 'dst', True, dstFormat='qcow2')
Line 187: 
Line 188:     def qcow2_compat_supported(self, cmd, **kw):
Lets not mix tests and helper functions - this should be after the tests.
Line 189:         self.check_supports_qcow2_compat(cmd, **kw)
Line 190:         return 0, 'Supported options:\ncompat ...\n', ''
Line 191: 
Line 192:     def test_qcow2_compat_supported(self):


Line 204:         expected = [QEMU_IMG, 'convert', '-O', 'qcow2', '-o', '?', '/dev/null']
Line 205:         self.assertEqual(cmd, expected)
Line 206: 
Line 207:     def test_qcow2_no_backing_file(self):
Line 208:         def qcow2_no_backing_file(cmd, **kw):
This repeats the name of the test, better use convert() as the other tests. If you think it is more clear, change all the tests, probably in another patch.
Line 209:             assert cmd == [qemuimg._qemuimg.cmd, 'convert', '-t', 'none',
Line 210:                            'source', '-O', 'qcow2', '-o', 'compat=0.10',
Line 211:                            'target']
Line 212:             return 0, '', ''


Line 206: 
Line 207:     def test_qcow2_no_backing_file(self):
Line 208:         def qcow2_no_backing_file(cmd, **kw):
Line 209:             assert cmd == [qemuimg._qemuimg.cmd, 'convert', '-t', 'none',
Line 210:                            'source', '-O', 'qcow2', '-o', 'compat=0.10',
If you like to have "source" and "target" instead of "src" and "dst" used in the other tests, change all the tests, in another patch probably.
Line 211:                            'target']
Line 212:             return 0, '', ''
Line 213: 
Line 214:         with FakeCmd(utils, 'execCmd', self.qcow2_compat_supported):


Line 215:             with FakeCmd(utils, 'watchCmd', qcow2_no_backing_file):
Line 216:                 qemuimg.convert('source', 'target', None, dstFormat='qcow2')
Line 217: 
Line 218:     def test_qcow2_backing_file(self):
Line 219:         def qcow2_backing_file(cmd, **kw):
Same repetition.
Line 220:             assert cmd == [qemuimg._qemuimg.cmd, 'convert', '-t', 'none',
Line 221:                            'source', '-O', 'qcow2', '-o',
Line 222:                            'compat=0.10,backing_file=backing', 'target']
Line 223:             return 0, '', ''


-- 
To view, visit http://gerrit.ovirt.org/33354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d6f254f5750fa249809469ecc764a73c91914d4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list