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

[aggpiyush]

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.

[aggpiyush]

@jleveque: Can you please review the changes?

[jleveque]

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.

[jleveque]

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

[aggpiyush]

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?

[jleveque]

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.

[lgtm-com]

This pull request introduces 1 alert when merging 1102843 into 295b68c - view on LGTM.com

new alerts:

• 1 for Unused local variable

[aggpiyush]

@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.

[jleveque]

We don't need to store this as a class member. It should be local only.

[jleveque]

Prefer using a more descriptive name, like eeprom_raw_bytes or similar.

[jleveque]

@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.

All methods which expect the full raw EEPROM bytes to be passed in should also be modified to default the parameter to None and then check if it is None, in which case it will use the new class member.

[aggpiyush]

@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]

@jleveque: The update consists of two changes.

1. Reverting the read_eeprom_bytes() return statement back to as it was.
2. Using more descriptive names of variables.

The error states -

ERROR: Could not find a version that satisfies the requirement sonic-yang-models>=1.0 (from sonic-config-engine) (from versions: none)
ERROR: No matching distribution found for sonic-yang-models>=1.0
##[error]Bash exited with code '1'.

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?