Martin Polednik has posted comments on this change. Change subject: vm: split guest agent init from domDependentInit ......................................................................
Patch Set 4: (1 comment) https://gerrit.ovirt.org/#/c/59816/4/vdsm/virt/vm.py File vdsm/virt/vm.py: PS4, Line 1850: self._dom_init_guest_agent() Not exactly first occurrence, but seeing this change, I'm a bit skeptical in regards to readability. Unless I'm missing something, the code in _dom_init_guest_agent could be restructured to display the logic in slightly more readable terms: try: self.guestAgent.start() except Exception: self.log.exception("Failed to connect to guest agent channel") else: guest_agent_started = True if guest_agent_started: try... (possibly omitted) Now, factoring the whole method out leaves no hint of the initialization logic. Have you considered only factoring out the method partially? try: self.guestAgent.start() except Exception: self.log.exception("Failed to connect to guest agent channel") else: _dom_trigger_migration_guest_event() # ?? finally: self.conf.pop('enableGuestEvents', None) Of course, this only works under assumption that the first restructuring is not wrong. What do you think - better/worse or doesn't fit further changes in the series? -- To view, visit https://gerrit.ovirt.org/59816 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I13e456d101659848c494f2c27912303c294ccf9f Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Milan Zamazal <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
