Nir Soffer has posted comments on this change. Change subject: storage: Initial size for thin provisioning disk ......................................................................
Patch Set 8: Code-Review+1 (1 comment) Looks good, but I want to discuss the ugly bytes/sectors interface first before we continue with this. https://gerrit.ovirt.org/#/c/46417/8/vdsm/storage/blockVolume.py File vdsm/storage/blockVolume.py: Line 177: Line 178: if initial_size and preallocate == volume.PREALLOCATED_VOL: Line 179: log.error("Initial size is not supported for preallocated volumes") Line 180: raise se.InvalidParameterException("initial size", Line 181: initial_size) This pattern of logging an error and raising is bad, leading to logging the same error twice. The first log give context, but does not show the value. The exception has no context but show the value. We should have (in a future patch), code like this: raise se.InvalidParameterException("Initial size is not supported for preallocated volumes", initial_size=initial_size) Logging this exception should show: InvalidParameterException: Initial size is not supported for preallocated volumes (initial_size=2048) Line 182: Line 183: if preallocate == volume.SPARSE_VOL: Line 184: if initial_size: Line 185: initial_size = int(initial_size * QCOW_OVERHEAD_FACTOR) -- To view, visit https://gerrit.ovirt.org/46417 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaf5f142541bf0b311a77a0544a1c4ffb689a9fde Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Fred Rolland <froll...@redhat.com> Gerrit-Reviewer: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Greg Padgett <gpadg...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
