Change in vdsm[master]: network: Introduce the network package

asegurap at redhat.com asegurap at redhat.com
Mon Mar 31 12:49:26 UTC 2014


Antoni Segura Puimedon has posted comments on this change.

Change subject: network: Introduce the network package
......................................................................


Patch Set 3:

(5 comments)

http://gerrit.ovirt.org/#/c/25908/3/vdsm/network/api.py
File vdsm/network/api.py:

Line 34: from .configurators import libvirt
Line 35: from .errors import ConfigNetworkError
Line 36: from . import errors as ne
Line 37: from .models import Bond, Bridge, IPv4, IPv6, IpConfig, Nic, Vlan
Line 38: import hooks  # TODO: This absolute import should go
> This should probably go before line 28.
Well, as it is now, it is a upper package import (except for the fact that vdsm/ is not yet a package so I can't do:
    from .. import hooks
It should be like that and that's why I put the TODO.
Line 39: 
Line 40: CONNECTIVITY_TIMEOUT_DEFAULT = 4
Line 41: 
Line 42: 


Line 675: 
Line 676: def usage():
Line 677:     print """Usage:
Line 678:     ./api.py add Network <attributes> <options>
Line 679:                        edit oldNetwork newNetwork <attributes> <options>
> These should be re-indented properly. Any plan (another patch) to move this
True! I should have re-indented it. Well, I would like to do it. I'll keep it in mind.
Line 680:                        del Network <options>
Line 681:                        setup Network [None|attributes] \
Line 682: [++ Network [None|attributes] [++ ...]] [:: <options>]
Line 683: 


http://gerrit.ovirt.org/#/c/25908/3/vdsm/network/configurators/ifcfg.py
File vdsm/network/configurators/ifcfg.py:

Line 16: #
Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: 
Line 20: from libvirt import libvirtError, VIR_ERR_NO_NETWORK
> Since this is not part of python and it's a "from" import it probably looks
Done
Line 21: import glob
Line 22: import logging
Line 23: import os
Line 24: import pipes


Line 38: from . import Configurator, getEthtoolOpts, libvirt
Line 39: from ..errors import ConfigNetworkError, ERR_FAILED_IFUP
Line 40: from ..models import Nic, Bridge, IpConfig
Line 41: from ..sourceroute import DynamicSourceRoute
Line 42: import dsaversion  # TODO: This absolute import from the package should go
> This should probably go before line 31.
This should be:
    from ... import dsaversion

Unfortunately vdsm/ is not yet a package so I can't do it. I'll try solving that first (or move dsaversion to lib/vdsm and then I'll change it here.
Line 43: 
Line 44: 
Line 45: def _hwaddr_required():
Line 46:     return config.get('vars', 'hwaddr_in_ifcfg') == 'always'


http://gerrit.ovirt.org/#/c/25908/3/vdsm/network/configurators/libvirt.py
File vdsm/network/configurators/libvirt.py:

Line 15: # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
Line 16: #
Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: from __future__ import absolute_import
> Why is this needed here and not it ifcfg.py as well?
Because here I want to do an absolute import but in ifcfg.py what I'm importing is the local version, i.e., this module, which happens to have what I want from it.

Maybe it'd be better if in ifcfg I import from the libvirt-python as well ;-)
Line 20: 
Line 21: from xml.dom.minidom import Document
Line 22: from xml.sax.saxutils import escape
Line 23: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I532c179d4002ed1e8e5445d02297c8d5a6248647
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani 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