Francesco Romani has posted comments on this change. Change subject: vm: move device configuration in a module ......................................................................
Patch Set 6: (5 comments) http://gerrit.ovirt.org/#/c/36266/6/vdsm/virt/vmdevices/configuration.py File vdsm/virt/vmdevices/configuration.py: Line 32: Line 33: Line 34: DEFAULT_BRIDGE = config.get("vars", "default_bridge") Line 35: Line 36: > I'm not sure if 'configuration' is the right name for the module, since e.g I concur that naming can be surprising but we're dealing with oVirt jargon here, I'm afraid. The original code was dealing with device parameters (often named 'device configuration') either sent explicitely as such by newer Engines or reconstructed by legacy parameters sent by really ancient Engines. It is only after the processing performed by this code that this data will be fed to Device* objects to create the proper device tree. So yes, we're on the middle ground between vmdevices and vm. I'm leaning towards vmdevices, with the not so-subtle intent to shed load from vm.py. All that said, 'configuration' is not the best name on earth, but can't think of anything fitter, yet. Suggestions? :) Line 37: DEVICE_MAPPING = ( Line 38: (hwclass.DISK, storage.Drive), Line 39: (hwclass.NIC, network.Interface), Line 40: (hwclass.SOUND, core.Sound), Line 47: (hwclass.CONSOLE, core.Console), Line 48: (hwclass.REDIR, core.Redir), Line 49: (hwclass.RNG, core.Rng), Line 50: (hwclass.SMARTCARD, core.Smartcard), Line 51: (hwclass.TPM, core.Tpm)) And you're right, strictly speaking this code doesn't belong here. Will see if I can split into another module. Line 52: Line 53: Line 54: def make(): Line 55: return dict((dev, []) for dev, _ in DEVICE_MAPPING) Line 216: Line 217: return confDrives Line 218: Line 219: Line 220: def default_disk_interface(arch): > why do we keep DEFAULT_BRIDGE on the top, but disk interfaces here? No good reason except copy/past laziness. Will change. Line 221: DEFAULT_DISK_INTERFACES = {caps.Architecture.X86_64: 'ide', Line 222: caps.Architecture.PPC64: 'scsi'} Line 223: return DEFAULT_DISK_INTERFACES[arch] Line 224: Line 221: DEFAULT_DISK_INTERFACES = {caps.Architecture.X86_64: 'ide', Line 222: caps.Architecture.PPC64: 'scsi'} Line 223: return DEFAULT_DISK_INTERFACES[arch] Line 224: Line 225: > The naming scheme doesn't work here compared to others: _removable_drives_f Nicer indeed. Will change. Line 226: def _legacy_drives(conf): Line 227: """ Line 228: Backward compatibility for qa scripts that specify direct paths. Line 229: """ Line 237: 'iface': 'ide', 'index': index, Line 238: 'truesize': 0}) Line 239: return legacies Line 240: Line 241: > same as above indeed Line 242: def _removable_drives(conf, arch): Line 243: removables = [{ Line 244: 'type': hwclass.DISK, Line 245: 'device': 'cdrom', -- To view, visit http://gerrit.ovirt.org/36266 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If9b133a75cd4cb5d11b1f09aff711846eca33ff8 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
