Skip to content

feat: Added support for TLV_CODE_VENDOR_NAME #542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gupurush wants to merge 2 commits into
base: master
Choose a base branch
from
+35 −0
Open

feat: Added support for TLV_CODE_VENDOR_NAME #542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
007e93b
feat: Added support for TLV_CODE_VENDOR_NAME
gupurush Feb 13, 2025
318b147
add vendor tests with new updated syseeprom contents
gupurush Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions sonic_platform_base/sonic_eeprom/eeprom_tlvinfo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,16 @@ def modelstr(self, e):
return super(TlvInfoDecoder, self).modelstr(e)

return t[2].decode("ascii")


def vendorstr(self, e):
Copy link
Copy Markdown

@FengPan-Frank FengPan-Frank Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls add test in https://github.com/sonic-net/sonic-platform-common/blob/cb5564c20ac74694f2391759f9235eee428a97d0/tests/eeprom_base_test.py

Copy link
Copy Markdown
Author

@gupurush gupurush Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests added as requested.

'''
Returns the value field of the Vendor Name TLV as a string
'''
(is_valid, t) = self.get_tlv_field(e, self._TLV_CODE_VENDOR_NAME)
if not is_valid:
return super(TlvInfoDecoder, self).vendorstr(e)
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vendorstr() falls back to super(...).vendorstr(e) when the TLV isn't present, but the base EepromDecoder/parent classes don't define vendorstr. This will raise AttributeError for EEPROMs that don't include _TLV_CODE_VENDOR_NAME. Consider returning a safe default (e.g., empty string) when the TLV is missing, or add a vendorstr() implementation to the parent class that returns a default value.

Suggested change
return super(TlvInfoDecoder, self).vendorstr(e)
# Parent class does not define vendorstr(); return a safe default
return ""

Copilot uses AI. Check for mistakes.
return t[2].decode("ascii")


def serial_number_str(self, e):
Expand Down
18 changes: 18 additions & 0 deletions tests/eeprom_base_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
TEST_PATH = os.path.dirname(os.path.abspath(__file__))
EEPROM_HEX_FILE_FULL_PATH = os.path.join(TEST_PATH, EEPROM_HEX_FILE)
EEPROM_SYMLINK_FULL_PATH = os.path.join(TEST_PATH, EEPROM_SYMLINK)
EEPROM_HEX_FILE_V2 = os.path.join(TEST_PATH, "syseeprom_v2.hex")
EEPROM_SYMLINK_V2 = os.path.join(TEST_PATH, "vpd_info_v2")
class TestEepromTlvinfo:

@classmethod
Expand Down Expand Up @@ -38,12 +40,15 @@ def setup_class(cls):
if not os.path.exists(os.path.dirname(EEPROM_HEX_FILE_FULL_PATH)):
assert False, "File {} is not exist".format(EEPROM_HEX_FILE_FULL_PATH)
subprocess.check_call(['/usr/bin/xxd', '-r', '-p', EEPROM_HEX_FILE_FULL_PATH, EEPROM_SYMLINK_FULL_PATH])
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new v2 EEPROM fixture is consumed via xxd without verifying the file exists first. Adding an explicit os.path.exists(EEPROM_HEX_FILE_V2) assertion (similar to the existing v1 check) would produce a clearer test failure than xxd's CalledProcessError if the fixture is missing or misnamed.

Suggested change
subprocess.check_call(['/usr/bin/xxd', '-r', '-p', EEPROM_HEX_FILE_FULL_PATH, EEPROM_SYMLINK_FULL_PATH])
subprocess.check_call(['/usr/bin/xxd', '-r', '-p', EEPROM_HEX_FILE_FULL_PATH, EEPROM_SYMLINK_FULL_PATH])
if not os.path.exists(EEPROM_HEX_FILE_V2):
assert False, "File {} is not exist".format(EEPROM_HEX_FILE_V2)

Copilot uses AI. Check for mistakes.
subprocess.check_call(['/usr/bin/xxd', '-r', '-p', EEPROM_HEX_FILE_V2, EEPROM_SYMLINK_V2])

@classmethod
def teardown_class(cls):
# Remove the mock eeprom after test
if os.path.exists(os.path.dirname(EEPROM_HEX_FILE_FULL_PATH)):
subprocess.check_call(['rm', '-f', EEPROM_SYMLINK_FULL_PATH])
if os.path.exists(os.path.dirname(EEPROM_HEX_FILE_V2)):
subprocess.check_call(['rm', '-f', EEPROM_SYMLINK_V2])

def test_eeprom_tlvinfo_read_api(self):
# Test using the api to fetch Base MAC, Switch Addr Range, Model,
Expand All @@ -57,6 +62,19 @@ def test_eeprom_tlvinfo_read_api(self):
assert(eeprom_class.serial_number_str(eeprom).rstrip('\0') == 'MT1623X09522')
assert(eeprom_class.part_number_str(eeprom).rstrip('\0') == 'MSN2700-CS2FO')
Comment on lines 53 to 63
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is coverage for the happy path where Vendor Name TLV exists, but no test exercises vendorstr() behavior when the TLV is absent (e.g., using the existing v1 syseeprom.hex). After fixing the fallback behavior, consider adding an assertion in test_eeprom_tlvinfo_read_api for the expected default (empty string/None/etc.) to prevent regressions.

Copilot uses AI. Check for mistakes.

def test_eeprom_tlvinfo_read_api_v2(self):
# Test using the updated api (addition of Vendor Name) to fetch Base MAC, Model, Vendor Name,
# Serial Number and Part Number.
eeprom_class = eeprom_tlvinfo.TlvInfoDecoder(EEPROM_SYMLINK_V2, 0, '', True)
eeprom = eeprom_class.read_eeprom()
eeprom_class.decode_eeprom(eeprom)
assert(eeprom_class.base_mac_addr(eeprom).rstrip('\0') == '34:73:2D:30:70:D8')
assert(eeprom_class.switchaddrrange(eeprom).rstrip('\0') == '516')
assert(eeprom_class.modelstr(eeprom).rstrip('\0') == '8102-64H-O')
assert(eeprom_class.vendorstr(eeprom).rstrip('\0') == 'Cisco')
assert(eeprom_class.serial_number_str(eeprom).rstrip('\0') == 'FLM251907U7')
assert(eeprom_class.part_number_str(eeprom).rstrip('\0') == 'ECI123')
Comment on lines +65 to +76
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description references dhclient.conf.j2 changes for DHCP option 60/61, but this repository/PR only appears to update the EEPROM TLV decoder and tests (no dhclient.conf.j2 present/modified here). Consider updating the PR description to match the actual scope of this repo change, or link to the other PRs where the DHCP config changes live.

Copilot uses AI. Check for mistakes.

def test_eeprom_tlvinfo_get_tlv_field(self):
# Test getting fields by field code
eeprom_class = eeprom_tlvinfo.TlvInfoDecoder(EEPROM_SYMLINK_FULL_PATH, 0, '', True)
Expand Down
7 changes: 7 additions & 0 deletions tests/syseeprom_v2.hex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
54 6c 76 49 6e 66 6f 00 01 00 5e 22 06 45 43 49
31 32 33 2a 02 02 04 23 0b 46 4c 4d 32 35 31 39
30 37 55 37 21 0a 38 31 30 32 2d 36 34 48 2d 4f
26 01 00 24 06 34 73 2d 30 70 d8 28 14 78 38 36
5f 36 34 2d 38 31 30 32 5f 36 34 68 5f 6f 2d 72
30 2b 05 43 69 73 63 6f 2d 05 43 69 73 63 6f 2c
02 55 53 fe 04 e0 30 3f 8e