api: HardwareVersion.IsSupported vs IsValid

This patch modifies the behavior of the HardwareVersion.IsValid function so that it returns true if the specified version can be parsed. There is a new function, HardwareVersion.IsSupported, that acts more like the old behavior of IsValid. IsSupported only returns true when the version is known to GoVmomi. BREAKING: HardwareVersion.IsValid is more relaxed, consider IsSupported * HardwareVersion.IsValid returns true if the specified value matches a VMX version format. * HardwareVersion.IsSupported behaves how IsValid used to behave.
vmware · May 20, 2024 · fc2a6b1 · fc2a6b1
1 parent e172af4
commit fc2a6b1
Show file tree

Hide file tree

Showing 2 changed files with 158 additions and 72 deletions.
diff --git a/vim25/types/hardware_version.go b/vim25/types/hardware_version.go
@@ -25,6 +25,10 @@ import (
 // HardwareVersion is a VMX hardware version.
 type HardwareVersion uint8
 
+const (
+	invalidHardwareVersion HardwareVersion = 0
+)
+
 const (
 	VMX3 HardwareVersion = iota + 3
 	VMX4
@@ -59,12 +63,24 @@ const (
 	MaxValidHardwareVersion = VMX21
 )
 
-func (hv HardwareVersion) IsValid() bool {
-	return hv != vmx5 &&
+// IsSupported returns true if the hardware version is known to and supported by
+// GoVmomi's generated types.
+func (hv HardwareVersion) IsSupported() bool {
+	return hv.IsValid() &&
+		hv != vmx5 &&
 		hv >= MinValidHardwareVersion &&
 		hv <= MaxValidHardwareVersion
 }
 
+// IsValid returns true if the hardware version is not valid.
+// Unlike IsSupported, this function returns true as long as the hardware
+// version is greater than 0.
+// For example, the result of parsing "abc" or "vmx-abc" is an invalid hardware
+// version, whereas the result of parsing "vmx-99" is valid, just not supported.
+func (hv HardwareVersion) IsValid() bool {
+	return hv != invalidHardwareVersion
+}
+
 func (hv HardwareVersion) String() string {
 	if hv.IsValid() {
 		return fmt.Sprintf("vmx-%d", hv)
@@ -85,7 +101,10 @@ func (hv *HardwareVersion) UnmarshalText(text []byte) error {
 	return nil
 }
 
-var vmxRe = regexp.MustCompile(`(?i)^vmx-(\d+)$`)
+var (
+	vmxRe        = regexp.MustCompile(`(?i)^vmx-(\d+)$`)
+	vmxNumOnlyRe = regexp.MustCompile(`^(\d+)$`)
+)
 
 // MustParseHardwareVersion parses the provided string into a hardware version.
 func MustParseHardwareVersion(s string) HardwareVersion {
@@ -97,25 +116,35 @@ func MustParseHardwareVersion(s string) HardwareVersion {
 }
 
 // ParseHardwareVersion parses the provided string into a hardware version.
+// Supported formats include vmx-123 or 123. Please note that the parser will
+// only return an error if the supplied version does not match the supported
+// formats.
+// Once parsed, use the function IsSupported to determine if the hardware
+// version falls into the range of versions known to GoVmomi.
 func ParseHardwareVersion(s string) (HardwareVersion, error) {
-	var u uint64
 	if m := vmxRe.FindStringSubmatch(s); len(m) > 0 {
-		u, _ = strconv.ParseUint(m[1], 10, 8)
-	} else {
-		u, _ = strconv.ParseUint(s, 10, 8)
-	}
-	v := HardwareVersion(u)
-	if !v.IsValid() {
-		return 0, fmt.Errorf("invalid version: %q", s)
+		u, err := strconv.ParseUint(m[1], 10, 8)
+		if err != nil {
+			return invalidHardwareVersion, fmt.Errorf(
+				"failed to parse %s from %q as uint8: %w", m[1], s, err)
+		}
+		return HardwareVersion(u), nil
+	} else if m := vmxNumOnlyRe.FindStringSubmatch(s); len(m) > 0 {
+		u, err := strconv.ParseUint(m[1], 10, 8)
+		if err != nil {
+			return invalidHardwareVersion, fmt.Errorf(
+				"failed to parse %s as uint8: %w", m[1], err)
+		}
+		return HardwareVersion(u), nil
 	}
-	return v, nil
+	return invalidHardwareVersion, fmt.Errorf("invalid version: %q", s)
 }
 
 var hardwareVersions []HardwareVersion
 
 func init() {
 	for i := MinValidHardwareVersion; i <= MaxValidHardwareVersion; i++ {
-		if i.IsValid() {
+		if i.IsSupported() {
 			hardwareVersions = append(hardwareVersions, i)
 		}
 	}

diff --git a/vim25/types/hardware_version_test.go b/vim25/types/hardware_version_test.go
@@ -25,40 +25,76 @@ import (
 
 func TestParseHardwareVersion(t *testing.T) {
 	testCases := []struct {
-		name            string
-		in              string
-		expectedIsValid bool
-		expectedVersion HardwareVersion
-		expectedString  string
+		name                string
+		in                  string
+		expectedIsValid     bool
+		expectedIsSupported bool
+		expectedVersion     HardwareVersion
+		expectedString      string
 	}{
 		{
-			name: "EmptyString",
-			in:   "",
+			name:                "EmptyString",
+			in:                  "",
+			expectedIsValid:     false,
+			expectedIsSupported: false,
+			expectedVersion:     0,
+			expectedString:      "",
 		},
 		{
-			name: "PrefixSansNumber",
-			in:   "vmx-",
+			name:                "PrefixSansNumber",
+			in:                  "vmx-",
+			expectedIsValid:     false,
+			expectedIsSupported: false,
+			expectedVersion:     0,
+			expectedString:      "",
 		},
 		{
-			name:            "NumberSansPrefix",
-			in:              "13",
-			expectedIsValid: true,
-			expectedVersion: VMX13,
-			expectedString:  "vmx-13",
+			name:                "NumberSansPrefix",
+			in:                  "13",
+			expectedIsValid:     true,
+			expectedIsSupported: true,
+			expectedVersion:     VMX13,
+			expectedString:      "vmx-13",
 		},
 		{
-			name:            "vmx-13",
-			in:              "vmx-13",
-			expectedIsValid: true,
-			expectedVersion: VMX13,
-			expectedString:  "vmx-13",
+			name:                "PrefixAndNumber",
+			in:                  "vmx-13",
+			expectedIsValid:     true,
+			expectedIsSupported: true,
+			expectedVersion:     VMX13,
+			expectedString:      "vmx-13",
 		},
 		{
-			name:            "VMX-18",
-			in:              "VMX-18",
-			expectedIsValid: true,
-			expectedVersion: VMX18,
-			expectedString:  "vmx-18",
+			name:                "UpperPrefixAndNumber",
+			in:                  "VMX-18",
+			expectedIsValid:     true,
+			expectedIsSupported: true,
+			expectedVersion:     VMX18,
+			expectedString:      "vmx-18",
+		},
+		{
+			name:                "vmx-99",
+			in:                  "vmx-99",
+			expectedIsValid:     true,
+			expectedIsSupported: false,
+			expectedVersion:     HardwareVersion(99),
+			expectedString:      "vmx-99",
+		},
+		{
+			name:                "ETooLarge",
+			in:                  "vmx-512",
+			expectedIsValid:     false,
+			expectedIsSupported: false,
+			expectedVersion:     0,
+			expectedString:      "",
+		},
+		{
+			name:                "ETooLarge2",
+			in:                  "512",
+			expectedIsValid:     false,
+			expectedIsSupported: false,
+			expectedVersion:     0,
+			expectedString:      "",
 		},
 	}
 
@@ -73,13 +109,14 @@ func TestParseHardwareVersion(t *testing.T) {
 				if a, e := v.IsValid(), tc.expectedIsValid; a != e {
 					t.Errorf("unexpected invalid value: a=%v, e=%v", a, e)
 				}
-				if v.IsValid() {
-					if a, e := v, tc.expectedVersion; a != e {
-						t.Errorf("unexpected value: a=%v, e=%v", a, e)
-					}
-					if a, e := v.String(), tc.expectedString; a != e {
-						t.Errorf("unexpected string: a=%v, e=%v", a, e)
-					}
+				if a, e := v.IsSupported(), tc.expectedIsSupported; a != e {
+					t.Errorf("unexpected supported value: a=%v, e=%v", a, e)
+				}
+				if a, e := v, tc.expectedVersion; a != e {
+					t.Errorf("unexpected value: a=%v, e=%v", a, e)
+				}
+				if a, e := v.String(), tc.expectedString; a != e {
+					t.Errorf("unexpected string: a=%v, e=%v", a, e)
 				}
 			})
 		}
@@ -92,11 +129,12 @@ func TestHardwareVersion(t *testing.T) {
 	var uniqueExpectedVersions []HardwareVersion
 
 	type testCase struct {
-		name            string
-		in              string
-		expectedIsValid bool
-		expectedVersion HardwareVersion
-		expectedString  string
+		name                string
+		in                  string
+		expectedIsValid     bool
+		expectedIsSupported bool
+		expectedVersion     HardwareVersion
+		expectedString      string
 	}
 
 	testCasesForVersion := func(
@@ -109,45 +147,64 @@ func TestHardwareVersion(t *testing.T) {
 		szVersion := strconv.Itoa(version)
 		return []testCase{
 			{
-				name:            szVersion,
-				in:              szVersion,
-				expectedIsValid: true,
-				expectedVersion: expectedVersion,
-				expectedString:  expectedString,
+				name:                szVersion,
+				in:                  szVersion,
+				expectedIsValid:     true,
+				expectedIsSupported: true,
+				expectedVersion:     expectedVersion,
+				expectedString:      expectedString,
 			},
 			{
-				name:            "vmx-" + szVersion,
-				in:              "vmx-" + szVersion,
-				expectedIsValid: true,
-				expectedVersion: expectedVersion,
-				expectedString:  expectedString,
+				name:                "vmx-" + szVersion,
+				in:                  "vmx-" + szVersion,
+				expectedIsValid:     true,
+				expectedIsSupported: true,
+				expectedVersion:     expectedVersion,
+				expectedString:      expectedString,
 			},
 			{
-				name:            "VMX-" + szVersion,
-				in:              "VMX-" + szVersion,
-				expectedIsValid: true,
-				expectedVersion: expectedVersion,
-				expectedString:  expectedString,
+				name:                "VMX-" + szVersion,
+				in:                  "VMX-" + szVersion,
+				expectedIsValid:     true,
+				expectedIsSupported: true,
+				expectedVersion:     expectedVersion,
+				expectedString:      expectedString,
 			},
 		}
 	}
 
 	testCases := []testCase{
 		{
-			name: "EmptyString",
-			in:   "",
+			name:                "EmptyString",
+			in:                  "",
+			expectedIsValid:     false,
+			expectedIsSupported: false,
+			expectedVersion:     0,
+			expectedString:      "",
 		},
 		{
-			name: "PrefixSansVersion",
-			in:   "vmx-",
+			name:                "PrefixSansVersion",
+			in:                  "vmx-",
+			expectedIsValid:     false,
+			expectedIsSupported: false,
+			expectedVersion:     0,
+			expectedString:      "",
 		},
 		{
-			name: "PrefixAndInvalidVersion",
-			in:   "vmx-0",
+			name:                "PrefixAndInvalidVersion",
+			in:                  "vmx-0",
+			expectedIsValid:     false,
+			expectedIsSupported: false,
+			expectedVersion:     0,
+			expectedString:      "",
 		},
 		{
-			name: "InvalidVersion",
-			in:   "1",
+			name:                "UnsupportedVersion",
+			in:                  "1",
+			expectedIsValid:     true,
+			expectedIsSupported: false,
+			expectedVersion:     HardwareVersion(1),
+			expectedString:      "vmx-1",
 		},
 	}