Skip to content
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

[sonic_eeprom/eeprom_base] EEPROM data instantiated and stored as a class member #190

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[sonic_eeprom/eeprom_base] EEPROM data instantiated and stored as a class member #190

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
446fb32
Update eeprom_base.py
May 30, 2021
1102843
Update eeprom_base.py
Jun 1, 2021
2604fb0
Update eeprom_base.py
Jun 3, 2021
31a4bf8
Update eeprom_base.py
Jun 14, 2021
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
15 changes: 10 additions & 5 deletions sonic_platform_base/sonic_eeprom/eeprom_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ def __init__(self, path, format, start, status, readonly):
self.r = readonly
self.cache_name = None
self.cache_update_needed = False
self.eeprom_file_handle = None
self.lock_file = None

def check_status(self):
Expand Down Expand Up @@ -220,7 +221,8 @@ def open_eeprom(self):
except Exception:
pass
self.cache_update_needed = using_eeprom
return io.open(eeprom_file, "rb")
self.eeprom_file_handle = io.open(eeprom_file, "rb")
return using_eeprom
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, changing the return value will break all existing code which is already using this library. These changes will need to be implemented incrementally. This PR should store the EEPROM file handle in a member variable as you have done, but it should not break existing functionality. All methods which currently take the EEPROM file handle as a parameter should be modified such that the parameter defaults to None, and if the handle is None, then the function should use self.eeprom_file_handle. This will allow vendors to update their existing code to stop passing the handle to methods. Once all vendors have transitioned, then we can change this return value to a boolean.

Copy link
Contributor

Choose a reason for hiding this comment

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

read_eeprom_bytes does not take the EEPROM file handle as a parameter. However, other methods do.

Copy link
Author

@aggpiyush aggpiyush Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm sorry but I don't see any methods where the file handle is passed as a parameter. It is used in many methods but not explicitly passed as a parameter. Can you please guide me through it?
Also, one of my concerns was regarding updating the following code segment:

if len(o) != byteCount and not self.cache_update_needed:
    os.remove(self.cache_name)
    self.cache_update_needed = True
    F.close()
    F = self.open_eeprom()
    F = self.eeprom_file_handle
    F.seek(self.s + offset)
    o = F.read(byteCount)

In this code segment, we call the 'open_eeprom( )' function to update the 'eeprom_file' path but since we are returning a file handle now, we have to store it again. So, to avoid this, do we explicitly pass the path info here or do we need to update the file handle again as in the code above so that while updating the code in the future, the function call can be omitted without any issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for confusion. It is not the file handle which is passed as a parameter, but rather the raw EEPROM contents returned by read_eeprom(). It seems unnecessary for the calling application to store this data and pass it back into class methods.


def read_eeprom(self):
sizeof_info = 0
Expand All @@ -231,8 +233,9 @@ def read_eeprom(self):

def read_eeprom_bytes(self, byteCount, offset=0):
F = None
eeprom_flag = False
try:
F = self.open_eeprom()
F = self.eeprom_file_handle
F.seek(self.s + offset)
o = F.read(byteCount)

Expand All @@ -243,9 +246,11 @@ def read_eeprom_bytes(self, byteCount, offset=0):
os.remove(self.cache_name)
self.cache_update_needed = True
F.close()
F = self.open_eeprom()
F.seek(self.s + offset)
o = F.read(byteCount)
eeprom_flag = self.open_eeprom()
if (eeprom_flag == self.cache_update_needed):
F = self.eeprom_file_handle
F.seek(self.s + offset)
o = F.read(byteCount)

if len(o) != byteCount:
raise RuntimeError("Expected to read %d bytes from %s, "
Expand Down