Change in vdsm[master]: storage: introducing vdsm-dump-chains script (part of vdsm-t...

nsoffer at redhat.com nsoffer at redhat.com
Tue Mar 31 22:00:36 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: storage: introducing vdsm-dump-chains script (part of vdsm-tool)
......................................................................


Patch Set 12:

(5 comments)

https://gerrit.ovirt.org/#/c/38281/12/debian/vdsm-python.install
File debian/vdsm-python.install:

Line 37: ./usr/lib/python2.7/dist-packages/vdsm/tool/upgrade.py
Line 38: ./usr/lib/python2.7/dist-packages/vdsm/tool/upgrade_300_networks.py
Line 39: ./usr/lib/python2.7/dist-packages/vdsm/tool/validate_ovirt_certs.py
Line 40: ./usr/lib/python2.7/dist-packages/vdsm/tool/vdsm-id.py
Line 41: ./usr/lib/python2.7/dist-packages/vdsm/tool/dump_volume_chains.py
Please keep this sorted, move bellow dump_bonding_defaults.py
Line 42: ./usr/lib/python2.7/dist-packages/vdsm/utils.py
Line 43: ./usr/lib/python2.7/dist-packages/vdsm/vdscli.py
Line 44: ./usr/lib/python2.7/dist-packages/vdsm/virtsparsify.py


https://gerrit.ovirt.org/#/c/38281/12/lib/vdsm/tool/Makefile.am
File lib/vdsm/tool/Makefile.am:

Line 51: 	unified_persistence.py \
Line 52: 	upgrade.py \
Line 53: 	upgrade_300_networks.py \
Line 54: 	vdsm-id.py \
Line 55: 	dump_volume_chains.py \
This should be sorted - move below dump_bonding_defaults.py
Line 56: 	$(NULL)
Line 57: 
Line 58: CLEANFILES = \
Line 59: 	config.log \


https://gerrit.ovirt.org/#/c/38281/12/lib/vdsm/tool/dump_volume_chains.py
File lib/vdsm/tool/dump_volume_chains.py:

Line 52:     pass
Line 53: 
Line 54: 
Line 55: class ChainError(DumpChainsError):
Line 56:     _chainError_message = """Error in volume chain %s: %s
Why _chainError_message? this is part of ChainError - there is not reason to call use a prefix.

If you don't like to call it message, call it "format" or "template"
Line 57:         volumes and parents: %s"""
Line 58: 
Line 59:     def __init__(self, volumes_children, img_uuid):
Line 60:         self.volumes_children = volumes_children


Line 60:         self.volumes_children = volumes_children
Line 61:         self.img_uuid = img_uuid
Line 62: 
Line 63:     def __str__(self):
Line 64:         volumes = ' '.join(['%s<-%s' % (parent, child) for parent, child in
This is impossible to read:

- We need spaces around the "<-"
- We cannot print multiple uuid pairs on the same line

This must be formatted one pair per line, and the indentation must be kept on the next lines.

This proves that the formatting does not belong to this error, but to the code formatting the output.
Line 65:                             self.volumes_children])
Line 66:         return ChainError._chainError_message % (
Line 67:             self.img_uuid, self.__class__.__name__, volumes)
Line 68: 


Line 62: 
Line 63:     def __str__(self):
Line 64:         volumes = ' '.join(['%s<-%s' % (parent, child) for parent, child in
Line 65:                             self.volumes_children])
Line 66:         return ChainError._chainError_message % (
Use self instead of ChainError.
Line 67:             self.img_uuid, self.__class__.__name__, volumes)
Line 68: 
Line 69: 
Line 70: class DuplicateParentError(ChainError):


-- 
To view, visit https://gerrit.ovirt.org/38281
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I428c443bb7d6b2a504a6f77efcd4838f7ae6c404
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Darshan N <dnarayan at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vladik Romanovsky <vladik.romanovsky at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yaniv Dary <ydary 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