Change in vdsm[master]: gluster: Get size information of a gluster volume.

barumuga at redhat.com barumuga at redhat.com
Wed Jun 11 09:56:07 UTC 2014


Bala.FA has posted comments on this change.

Change subject: gluster: Get size information of a gluster volume.
......................................................................


Patch Set 2: Code-Review-1

(7 comments)

http://gerrit.ovirt.org/#/c/28581/2/vdsm/gluster/api.py
File vdsm/gluster/api.py:

Line 307:         return {'tasks': status}
Line 308: 
Line 309:     @exportAsVerb
Line 310:     def volumeStatsInfoGet(self, volumeName, options=None):
Line 311:         return self.svdsmProxy.glusterVolumeStatsInfoGet(volumeName)
Any plans to give inode used/free/total too?
Line 312: 
Line 313: 
Line 314: def getGlusterMethods(gluster):
Line 315:     l = []


http://gerrit.ovirt.org/#/c/28581/2/vdsm/gluster/gfapi.py
File vdsm/gluster/gfapi.py:

Line 60:                            use_errno=True)
Line 61:     return
Line 62: 
Line 63: 
Line 64: def _volumeMount(volumeId):
you could have this function name as glfsInit() as per libfgapi
Line 65:     _glfs_new = ctypes.CFUNCTYPE(
Line 66:         ctypes.c_void_p, ctypes.c_char_p)(('glfs_new', _api))
Line 67:     fs = _glfs_new(volumeId)
Line 68: 


Line 88:             errMsg += "Volume is stopped."
Line 89:         elif rc == -1:
Line 90:             errMsg += "Volume not found."
Line 91:         raise ge.GlusterVolumeGlfsInitFailedException(rc=rc,
Line 92:                                                       err=[errMsg])
1. This nested if can be improved like
if rc == 0:
    return fs
elif rc == 1:
    raise ge.expection(...)
elif rc == -1:
    raise ge.expection(...)
else:
    raise ge.expection(...)  # this is unknown


2. I feel we could have one exception for this function like
ge.GlfsInitException()
Line 93:     return fs
Line 94: 
Line 95: 
Line 96: def _volumeUmount(fs, volumeId):


Line 92:                                                       err=[errMsg])
Line 93:     return fs
Line 94: 
Line 95: 
Line 96: def _volumeUmount(fs, volumeId):
you could have this function name as glfsFini() as per libfgapi
Line 97:     _glfs_fini = ctypes.CFUNCTYPE(
Line 98:         ctypes.c_int, ctypes.c_void_p)(('glfs_fini', _api))
Line 99:     rc = _glfs_fini(fs)
Line 100:     if rc != 0:


Line 97:     _glfs_fini = ctypes.CFUNCTYPE(
Line 98:         ctypes.c_int, ctypes.c_void_p)(('glfs_fini', _api))
Line 99:     rc = _glfs_fini(fs)
Line 100:     if rc != 0:
Line 101:         raise ge.GlusterVolumeGlfsFinitFailedException(rc=rc)
I feel we could have better name of exception like ge.GlfsFiniException()
Line 102: 
Line 103: 
Line 104: def volumeStatvfs(volumeId):
Line 105:     statvfsdata = StatVfsStruct()


Line 100:     if rc != 0:
Line 101:         raise ge.GlusterVolumeGlfsFinitFailedException(rc=rc)
Line 102: 
Line 103: 
Line 104: def volumeStatvfs(volumeId):
Please accept optional arg host, port and protocol.  This will wide up the usage.
Line 105:     statvfsdata = StatVfsStruct()
Line 106:     _lazyInitGfapi()
Line 107: 
Line 108:     # Define return type and argtypes of glfs_statvfs


Line 139:                               statvfsdata.f_namemax))
Line 140: 
Line 141: 
Line 142: @makePublic
Line 143: def volumeStatsInfoGet(volumeName):
Please move this function logic to api.volumeStatInfoGet()
Line 144: 
Line 145:     data = volumeStatvfs(volumeName)
Line 146: 
Line 147:     # f_blocks = Total number of blocks


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If5d622738ae955eb4002f56fc73adb4f07f0b857
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Darshan N <dnarayan at redhat.com>
Gerrit-Reviewer: Aravinda VK <avishwan at redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga at redhat.com>
Gerrit-Reviewer: Darshan N <dnarayan at redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin 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