Douglas Schilling Landgraf has posted comments on this change. Change subject: vdsm: Adding guest agent API versioning support ......................................................................
Patch Set 5: (5 comments) http://gerrit.ovirt.org/#/c/17004/5/vdsm/guestIF.py File vdsm/guestIF.py: Line 98: MAX_MESSAGE_SIZE = 2 ** 20 # 1 MiB for now Line 99: Line 100: def __init__(self, socketName, channelListener, log, user='Unknown', Line 101: ips='', connect=True): Line 102: self.apiVersion = -1 -1 means disable right? Maybe a constant here too. Line 103: self.log = log Line 104: self._socketName = socketName Line 105: self._sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) Line 106: self._stopped = True Line 142: Line 143: Note: Line 144: We do not return -1 since it means it's disabled and would be an Line 145: unexpected value for the protocol. (The value returned here is used Line 146: in the API) Re-reading the code this "Note" shouldn't be in _setAPIVersion? Since int(version) might return -1. Line 147: """ Line 148: _LOWEST_POSSIBLE_API_VERSION = 0 Line 149: try: Line 150: return int(version) Line 155: def _setAPIVersion(self, version): Line 156: version = self._validateAPIVersion(version) Line 157: if version > _MAX_SUPPORTED_API_VERSION: Line 158: version = _MAX_SUPPORTED_API_VERSION Line 159: elif version < 0: Maybe we should use _LOWEST_POSSIBLE_API_VERSION constant here? Another question, _MAX_SUPPORTED_API_VERSION = 0 shouldn't be _MIN_SUPPORED_API_VERSION? If yes, we could even remove _LOWEST_POSSIBLE_API_VERSION and use it too. Please note, I am not very familiar with this part of code, just trying to help. Line 160: version = 0 Line 161: if self.apiVersion != version: Line 162: self.log.info("Guest API version changed from %d to %d", Line 163: self.apiVersion, version) Line 224: self.log.debug("Common API version changed from %d to %d", Line 225: self.apiVersion, commonVersion) Line 226: self._forward('api-version', Line 227: {'apiVersion': commonVersion}) Line 228: elif self.apiVersion != -1: if we are going to use constant, could replace it here too. Line 229: self.log.debug("API versioning no longer reported by guest.") Line 230: self.apiVersion = -1 Line 231: elif message == 'host-name': Line 232: self.guestInfo['guestName'] = args['name'] Line 226: self._forward('api-version', Line 227: {'apiVersion': commonVersion}) Line 228: elif self.apiVersion != -1: Line 229: self.log.debug("API versioning no longer reported by guest.") Line 230: self.apiVersion = -1 here too Line 231: elif message == 'host-name': Line 232: self.guestInfo['guestName'] = args['name'] Line 233: elif message == 'os-version': Line 234: self.guestInfo['guestOs'] = args['version'] -- To view, visit http://gerrit.ovirt.org/17004 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9095b528c2c910f12d5f170088a458bf11c71910 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Douglas Schilling Landgraf <dougsl...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Peter V. Saveliev <p...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
