Change in vdsm[master]: janitorial: add utility to overwrite error message

nsoffer at redhat.com nsoffer at redhat.com
Thu Feb 19 16:01:49 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: janitorial: add utility to overwrite error message
......................................................................


Patch Set 1:

(3 comments)

Nice! but I think we need bigger change.

http://gerrit.ovirt.org/#/c/37912/1/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 1242:                      for _ in range(suffix_len))
Line 1243:     return prefix + suffix
Line 1244: 
Line 1245: 
Line 1246: def error_with_message(code, message):
error_response(name, message) ?

But lets not add more to the utils junkyard. How about moving the relevant code to response module:

# response.py
def error(name, message=None):
    ...

def ok():
    return {"status": {"code": ..., "message": ...}}
Line 1247:     return {'status':
Line 1248:             {'code': errCode[code]['status']['code'],


http://gerrit.ovirt.org/#/c/37912/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 3715:                 diskelem.toxml(), libvirt.VIR_DOMAIN_DEVICE_MODIFY_FORCE)
Line 3716:         except Exception:
Line 3717:             self.log.debug("updateDeviceFlags failed", exc_info=True)
Line 3718:             self.cif.teardownVolumePath(drivespec)
Line 3719:             return errCode['changeDisk']
I think we should change all these use the error response function:

    return response.error("changeDisk")

The code should be consistent, use same way to return a response/error. Then we can change the response format by modifying one place.
Line 3720:         if vmDev in self.conf:
Line 3721:             self.cif.teardownVolumePath(self.conf[vmDev])
Line 3722: 
Line 3723:         self.conf[vmDev] = path


Line 3746:         except TimeoutError as tmo:
Line 3747:             res = utils.error_with_message('ticketErr', unicode(tmo))
Line 3748:         else:
Line 3749:             hooks.after_vm_set_ticket(self._domain.xml, self.conf, params)
Line 3750:             res = {'status': doneCode}
Same for this - should be a function to generate an ok response.

    return response.OK
Line 3751:         return res
Line 3752: 
Line 3753:     def _reviveTicket(self, newlife):
Line 3754:         """


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife293080c2c6654315a609478f3276c9008ea6e7
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei 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