On Tue, Aug 30, 2011 at 10:11:31AM +0200, Michael Hanselmann wrote: > Am 29. August 2011 18:21 schrieb Agata Murawska <[email protected]>: > > + if network_ganeti_data: > > + ganeti_data["mode"] = network_ganeti_data.findtext("{%s}Mode" % > > + GANETI_SCHEMA) > > constants.INIC_MODE > > > + ganeti_data["mac"] = network_ganeti_data.findtext("{%s}MACAddress" > > % > > + GANETI_SCHEMA) > > constants.INIC_MAC > > > + ganeti_data["ip"] = network_ganeti_data.findtext("{%s}IPAddress" % > > + GANETI_SCHEMA) > > constants.INIC_IP > > > + ganeti_data["link"] = network_ganeti_data.findtext("{%s}Link" % > > + GANETI_SCHEMA) > > constants.INIC_LINK > > > + data = {} > > + if network_data: > > + data["mac"] = network_data.findtext("{%s}Address" % RASD_SCHEMA) > > Why use a dictionary instead of a single variable? > > > + network_name = network_name.lower() > > + > > + if constants.NIC_MODE_BRIDGED in network_name: > > + results["nic%s_mode" % str(counter)] = "bridged" > > No need for str() if you use %s. constants.NIC_MODE_BRIDGED > > > + elif constants.NIC_MODE_ROUTED in network_name: > > + results["nic%s_mode" % str(counter)] = "routed" > > constants.NIC_MODE_ROUTED > > You get the idea. There are other places where you should use existing > constants.
While this is a very nice cleanup, the problem is that we don't have a clear set of constants. We have constants for the keys as we use them in the config file. We have constants for the keys as we use them to interact with the CLI, and some overlap but not all. We need to introduce a separate set of constants for the backup, or declare that the backup always uses the CLI constants, but we can't simply choose INIC_MODE while also choosing NIC_MODE_ROUTED. I'm tempted to say that this cleanup is good but should be done separately from ovf, when we cleanup the existing import/export code too. iustin
