Change in vdsm[master]: clientIF: prepareVolumePath payload cleanup

nsoffer at redhat.com nsoffer at redhat.com
Wed Mar 19 16:08:55 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................


Patch Set 4: Code-Review-1

(1 comment)

Some parts became worse then the original code in latest patches.

http://gerrit.ovirt.org/#/c/24636/4/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 337:             return supervdsm.getProxy().mkIsoFs(vmId, payload['file'],
Line 338:                                                 payload.get('volId'))
Line 339:         elif device == 'floppy':
Line 340:             return supervdsm.getProxy().mkFloppyFs(vmId, payload['file'],
Line 341:                                                    payload.get('volId'))
The only difference in these calls is the function name but you repeat everything for no good reason. The old code (lines 299-310) was better.

Cleaning up the old code, we can do this:

    funcs = {'cdrom': 'mkIsoFs', 'floppy': 'mkFloppyFs'}

    try:
        func = getattr(supervdsm.getProxy(), funcs[device])
    except AttributeError:
        raise vm.VolumeError...

    return func(vmId, payload['file'], payload.get('volId'))

In another patch, we can improve this so we don't call the proxy to report unsupported device:

    funcs = {'cdrom': 'mkIsoFs', 'floppy': 'mkFloppyFs'}
    if device not in funcs:
        raise vm.VolumeError...

    func = getattr(supervdsm.getProxy(), funcs[device])
    return func(vmId, payload['file'], payload.get('volId'))

The second version will not raise KeyError on bad input, and AttributeError from the proxy is impossible since it implements both functions. But this changes the behavior and does not belong in this refactoring patch.
Line 342:         else:
Line 343:             raise vm.VolumeError("Unsupported 'device': %s" % device)
Line 344: 
Line 345:     def teardownVolumePath(self, drive):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I058206b7506ddbb5ec087c9ea0963a10ed57affb
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpoledni at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr 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