Giuseppe Vallarelli has posted comments on this change. Change subject: Qos feature: Adds bandwidth definition at configuration level. ......................................................................
Patch Set 3: (4 inline comments) Dan, it's not a big deal for me to use minidom but I feel that's not a good choice that's why I didn't use it in the first place. .................................................... File vdsm_api/vdsmapi-schema.json Line 168: # @onboot: #optional Start the network device automatically during boot Line 169: # Line 170: # @remove: #optional If True, remove existing network only Line 171: # Line 172: # @qosInbound: #optional VmInterfaceDeviceBandwidthParams for incoming traffic. I'm fine with BandwidthParams, so it's decided ;-) Line 173: # Line 174: # @qosOutbound: #optional VmInterfaceDeviceBandwidthParams for outgoing traffic. Line 175: # Line 176: # Since: 4.10.0 .................................................... File vdsm/netconf/ifcfg.py Line 17: # Refer to the README and COPYING files for full details of the license Line 18: # Line 19: Line 20: from xml.sax.saxutils import escape Line 21: from xml.etree.ElementTree import Element as XmlElement, tostring I didn't use minidom because I think that the usage of ElementTree API makes the code more readable, also is a widely accepted API. I know the importance of having a consistent codebase, but I consider the usage of minidom a questionable choice, I would much be in favor of refactoring and using ElementTree instead of baking up our own classes see XMLElement in vm module to cover minidom shortcomings. The best choice would be to use lxml (http://lxml.de/) - which is the most full featured and fast python library for dealing with xml but that require a team discussion, it implements the same API ElementTree. What do you think ? p.s. this is a classic Duty call http://xkcd.com/386/ Line 22: import glob Line 23: import libvirt Line 24: import logging Line 25: import os Line 245: net = conn.networkDefineXML(netXml) Line 246: net.create() Line 247: net.setAutostart(1) Line 248: Line 249: def createNetXml(self, netName, network, bridged, iface, I didn't prefix _ simply because I'm treating it as a public interface method being called from the test module, but I don't mind updating the name if you feel that's important. Line 250: qosInbound=None, qosOutbound=None): Line 251: """ Line 252: Creates Network Xml e.g.: Line 253: <network> Line 598: onboot='yes', **kwargs): Line 599: """ Create ifcfg-* file with proper fields for VLAN """ Line 600: conf = 'VLAN=yes\n' Line 601: if bridge: Line 602: conf += 'BRIDGE=%s\n' % pipes.quote(bridge) ops Line 603: self._createConfFile(conf, vlan, ipaddr, netmask, gateway, Line 604: bootproto, mtu, onboot, **kwargs) Line 605: Line 606: def addBonding(self, bonding, bridge=None, bondingOptions=None, mtu=None, -- To view, visit http://gerrit.ovirt.org/15724 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0c3b4b9a6fd5eb53579fb2e157b5325caa88d04 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Giuseppe Vallarelli <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
