[sonic_eeprom/eeprom_base] EEPROM data instantiated and stored as a class member #190
Conversation
|
@jleveque: Can you please review the changes? |
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
read_eeprom_bytes does not take the EEPROM file handle as a parameter. However, other methods do.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Updated for incremental approach.
|
This pull request introduces 1 alert when merging 1102843 into 295b68c - view on LGTM.com new alerts:
|
|
@jleveque: From what I understood, I modified the code to meet the requirements. Can you please check if the update suffices all the requirements? If not, please let me know the rest of the areas that require changes. |
| @@ -256,8 +261,8 @@ def read_eeprom_bytes(self, byteCount, offset=0): | |||
| finally: | |||
| if F is not None: | |||
| F.close() | |||
|
|
|||
| return bytearray(o) | |||
| self.byte_array = bytearray(o) | |||
There was a problem hiding this comment.
We don't need to store this as a class member. It should be local only.
| @@ -29,6 +29,9 @@ 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.o = None | |||
There was a problem hiding this comment.
Prefer using a more descriptive name, like eeprom_raw_bytes or similar.
All methods which expect the full raw EEPROM bytes to be passed in should also be modified to default the parameter to |
|
@jleveque: Sorry for pestering you again but this is my first contribution to the open-source community. Hope you understand! I'm not able to understand what methods are you referring to when you say 'methods which expect the full raw EEPROM bytes to be passed in'. If you can please provide me with an example or mention the methods that require the changes, I'll be able to understand better and get the rest of the work done. Thank you. |
aggpiyush
changed the title
|
@jleveque: The update consists of two changes.
The error states - Is this because of the changes made by me in the eeprom_base.py file or because of some other dependencies issues as the error states? |
Description
open_eeprom( ): Initially the function returned a file handle which was passed back into most of the methods in the class. Now, the file handle is declared with a global scope and stored internally in the function, there is no need for other functions to first store the file handle and then access it.
The function now returns a bool value 'using_eeprom' which indicates whether the file is read from cache or the path specified for the EEPROM file.
read_eeprom_bytes( ): The open_eeprom( ) function is referred twice in this function. In the first reference, simply the file handle is passed and accessed. In the second reference, a new bool variable 'eeprom_flag' is defined. Since the open_eeprom( ) function returns the flag 'using_eeprom' indicating whether the path specified for EEPROM is taken or not, it ensures that after removing the 'cache_name' file path from the OS, the file path is set to retrieve the file contents from the path specified for the EEPROM file.
Motivation and Context
" Fixes #170 "
Initially, the code was defeating the purpose of object-oriented programming. Now, necessary changes are made to resolve the issue.
How Has This Been Tested?
The testing has been done in the user's local system by trying mimicking the environment and its functionalities.