Change in vdsm[master]: hooks: Add fcoe hook

phoracek at redhat.com phoracek at redhat.com
Thu Apr 7 12:27:48 UTC 2016


Petr Horáček has posted comments on this change.

Change subject: hooks: Add fcoe hook
......................................................................


Patch Set 18: Code-Review-1

(10 comments)

most of them are 'code typography', i won't fight for them

https://gerrit.ovirt.org/#/c/55029/18/vdsm_hooks/fcoe/README
File vdsm_hooks/fcoe/README:

PS18, Line 7:  --cver=3.6
i don't really mind, but if you drop --cver opt, engine-config will ask user to select version and we won't have to update this file with 4.0


PS18, Line 10:   
double space (i see a double space, i can't let it be)


PS18, Line 10:                                 
nasty trailing whitespace


Line 7:   sudo engine-config -s UserDefinedNetworkCustomProperties='fcoe=^(true|false)$' --cver=3.6
Line 8: 
Line 9: * Verify that custom property was added:
Line 10:   engine-config  -g UserDefinedNetworkCustomProperties                                
Line 11: 
engine needs to be restarted so changes take place
Line 12: Usage:
Line 13: * Define virtual network for FCoE
Line 14: * Under "setup host networks" attach (drag) it to interface.
Line 15: * Click small "Edit" button and add fcoe property with value "true"


Line 14: * Under "setup host networks" attach (drag) it to interface.
Line 15: * Click small "Edit" button and add fcoe property with value "true"
Line 16: 
Line 17: 
Line 18: 
pending lines


https://gerrit.ovirt.org/#/c/55029/18/vdsm_hooks/fcoe/fcoe_before_network_setup.py
File vdsm_hooks/fcoe/fcoe_before_network_setup.py:

Line 27: def _has_fcoe_param(net_attr):
Line 28:     """
Line 29:     Check if fcoe parameter was specified for net
Line 30:     """
Line 31:     return net_attr.get('custom') \
you should use parantheses instead of \ if it is possible.

for example:

    return (net_attr.get('custom') and
            and hooking.tobool(net_attr['custom'].get('fcoe')))
Line 32:         and hooking.tobool(net_attr['custom'].get('fcoe'))
Line 33: 
Line 34: 
Line 35: def _configure(interface):


PS18, Line 36:     """
             :     Enable FCoE on specified interface by coping default configuration
             :     """
I think you should fit in on one line:
"""doc string"""


PS18, Line 39:     # IOError can be raised here is FCOE_DEFAULT_CONFIG doesn't exist
             :     # or we're not able to copy file
             :     # Don't catch exceptions here. Propagate them to main
             :     # and show traceback in supervdsm log
i feel bad for nagging for code typography but i cannot not comment this.

please fit the full width of the line if possible and does not make sense to create a new line.


Line 50: 
Line 51: 
Line 52: def _unconfigure(interface):
Line 53:     """
Line 54:     Remove config file for specified interface
the same here.
Line 55:     """
Line 56:     if interface is None:
Line 57:         return
Line 58:     filename = os.path.join(FCOE_CONFIG_DIR, 'cfg-%s' % (interface))


PS18, Line 90:         elif hooking.tobool(net_attr.get('remove')):
you don't have to use tobool, as we canonicalize values before we expose them to the hook.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iad2faed7205ca08801132df072b469dbe781318c
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Pavel Zhukov <pzhukov at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Pavel Zhukov <pzhukov at redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list