Change in vdsm[master]: net: Mapping bonding option value names to numerics

edwardh at redhat.com edwardh at redhat.com
Wed Dec 9 01:13:10 UTC 2015


Edward Haas has posted comments on this change.

Change subject: net: Mapping bonding option value names to numerics
......................................................................


Patch Set 12:

(10 comments)

https://gerrit.ovirt.org/#/c/49390/12/init/vdsmd_init_common.sh.in
File init/vdsmd_init_common.sh.in:

Line 152:         "@MOUNTPOINT_PATH@" -q /sys/fs/cgroup/systemd
Line 153: }
Line 154: 
Line 155: 
Line 156: task_dump_bonding_defaults(){
> should not we rename the function as well (and corresponding service)?
Done
But there is no service running it today. Probably needs to be fixed in a different patch.
Line 157:     "${VDSM_TOOL}" dump-bonding-options
Line 158: }
Line 159: 
Line 160: 


https://gerrit.ovirt.org/#/c/49390/12/lib/vdsm/netinfo.py
File lib/vdsm/netinfo.py:

Line 212:              if p != bond_mode_path)
Line 213:     opts = {}
Line 214: 
Line 215:     for path in paths:
Line 216:         element = _bond_opts_read_elements(path)
> maybe elements?
Done
Line 217:         if len(element) == 2:
Line 218:             opts[os.path.basename(path)] = \
Line 219:                 _bond_opts_name2numeric_scan(path)
Line 220:     return opts


Line 226:         for numeric_val in range(32):
Line 227:             name, numeric = _bond_opts_name2numeric_getval(opt_path, opt_file,
Line 228:                                                            numeric_val)
Line 229:             if name is None:
Line 230:                 break
> if name is not None:
The idea is to break from the for loop on the first fail to get the next value (because there are no more values).
Line 231: 
Line 232:             vals[name] = numeric
Line 233: 
Line 234:     return vals


Line 233: 
Line 234:     return vals
Line 235: 
Line 236: 
Line 237: def _bond_opts_name2numeric_getval(opt_path, opt_wr_file, numeric_val):
> +1
Ok. Just that opt_write_file.write looks a bit funny.
Done.
Line 238:     try:
Line 239:         opt_wr_file.write(str(numeric_val))
Line 240:         opt_wr_file.flush()
Line 241:     except IOError as e:


Line 253:     Return a dictionary in the same format as _bond_opts_name2numeric().
Line 254:     Exclude entries that are not bonding options,
Line 255:     e.g. 'ad_num_ports' or 'slaves'.
Line 256:     """
Line 257: 
> extra line
Done
Line 258:     return dict(((opt, val) for (opt, val)
Line 259:                  in _bond_opts_name2numeric(bond).iteritems()
Line 260:                  if opt not in _EXCLUDED_BONDING_ENTRIES))
Line 261: 


https://gerrit.ovirt.org/#/c/49390/12/lib/vdsm/tool/dump_bonding_opts.py
File lib/vdsm/tool/dump_bonding_opts.py:

Line 51:                     sort_keys=True, indent=4, separators=(',', ': '))
Line 52:     with open(BONDING_DEFAULTS, 'w') as defaults:
Line 53:         jdump(_get_default_bonding_options(), defaults)
Line 54: 
Line 55:     with open(BONDING_NAME2NUMERIC_PATH, 'w') as defaults:
> should not this be named name2numeric instead of defaults or something?
Changing it to f, it is close located and used once per context.
Line 56:         jdump(_get_bonding_options_name2numeric(), defaults)
Line 57: 
Line 58: 
Line 59: def _get_default_bonding_options():


Line 70: bond_opts_name2numeric_filtered(bond_name)
It needs to be bondOpts


Line 72: default_mode = bondOpts(bond_name, keys=['mode'])['mode']
Move it out of the for loop.


Line 90: 
Line 91: 
Line 92: @contextmanager
Line 93: def _bond_device(bond_name):
Line 94: 
> extra line
Done
Line 95:     with open(BONDING_MASTERS, 'w') as bonds:
Line 96:         bonds.write('+' + bond_name)
Line 97: 
Line 98:     try:


Line 104: 
Line 105: def _scan_modes(bond_name):
Line 106:     for mode in range(_MAX_BONDING_MODES + 1):
Line 107:         mode = str(mode)
Line 108:         with open(BONDING_OPT % (bond_name, 'mode'), 'w') as opt:
> open it once and then write everything in on cycle? maybe it would be faste
Yes, we will have to flush it each time.
Leaving the file open will also cause it to be opened from several modules in parallel, it may be hard in the future to track it.
This is intended as an one time job in production, IMO the time issue is negligible.
Line 109:             opt.write(mode)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f4c16b1523822a2d53d4525841ff8741af6296c
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: Sagi Shnaidman <sshnaidm at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list