Change in vdsm[master]: vdsm: Adding guest agent API versioning support

vfeenstr at redhat.com vfeenstr at redhat.com
Wed Jan 15 09:41:40 UTC 2014


Vinzenz Feenstra has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................


Patch Set 6:

(4 comments)

http://gerrit.ovirt.org/#/c/17004/6/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 26: import supervdsm
Line 27: import unicodedata
Line 28: 
Line 29: _MAX_SUPPORTED_API_VERSION = 0
Line 30: _MIN_SUPPORTED_API_VERSION = 0
> Hi,
Because with this patchset there's no new version, 0 is the baseline for now. There's a new patch in the pipe which will increase MAX to 1. Simply 0 means all what was there before. Since there's no versioning so far, this is simply version 0.

See here: http://gerrit.ovirt.org/#/c/23258/1/vdsm/guestIF.py
Line 31: _API_DISABLED_VALUE = -1
Line 32: 
Line 33: __REPLACEMENT_CHAR = u'\ufffd'
Line 34: __RESTRICTED_CHARS = set(range(8 + 1)). \


Line 138:         Args:
Line 139:         version - the api version value to validate
Line 140: 
Line 141:         Returns:
Line 142:         A valid api version or 0 which means the lowest possible version of
> Can return -1 (disable) at this stage too right?
no, it should not
Line 143:         the API.
Line 144:         """
Line 145:         try:
Line 146:             apiVersion = int(version)


Line 146:             apiVersion = int(version)
Line 147:         except ValueError:
Line 148:             self.log.warning("Received invalid version value: %s", version)
Line 149:             apiVersion = _MIN_SUPPORTED_API_VERSION
Line 150:         return min(apiVersion, _MIN_SUPPORTED_API_VERSION)
> Question: apiversion could be a value > 0? If yes, not sure if we should us
you're right, It actually MUST be a value >= 0.  This should have been max and not min so that at a very minimum it is _MIN_SUPPORTED_API_VERSION
Line 151: 
Line 152:     def _setAPIVersion(self, version):
Line 153:         """ Sets the API version if it was changed
Line 154:         Args:


Line 154:         Args:
Line 155:         version - the api version to set
Line 156: 
Line 157:         Note: If the value of `version` is not in between
Line 158:         _MIN_SUPPORTED_API_VERSION and _MAX_SUPPORTED_API_VERSION the value
> between? but both init as 0, not sure if I understood it.
Well that's the documentation, it doesn't matter that there's no version 1 yet, however this will come with the above mentioned patchset.
Well this is the documentation, the constants can change over time.
Line 159:         will be adjusted to have the highest possible API version supported.
Line 160: 
Line 161:         This should not be required and the protocol does not rely on this,
Line 162:         this is simply a data santization fallback.


-- 
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: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Sivák <msivak at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Peter V. Saveliev <peet at redhat.com>
Gerrit-Reviewer: Roy Golan <rgolan at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list