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

Jump to bottom

Fix double hex to decimal conversion #3267

Merged
merged 1 commit into from
Apr 19, 2024
Merged

Fix double hex to decimal conversion #3267

merged 1 commit into from
Apr 19, 2024

Conversation

yuazhe
Copy link
Contributor

@yuazhe yuazhe commented Apr 10, 2024 •
edited
Loading

What I did

In the previous commit with hash a3cf5c #3153 that aimed to address the issue where sfputil incorrectly interpreted page numbers as decimal instead of hexadecimal, there was an inadvertent double conversion from hexadecimal to decimal. For instance, inputting 11 resulted in conversion to 17 and then further to 23. To rectify this, the second conversion would be removed.

How I did it

remove the redundant second conversion

How to verify it

check the eeprom-hexdump output
A related UT has been added

Previous command output (if the output of a command-line utility has changed)

admin@sonic:~$ sudo sfputil show eeprom-hexdump -p Ethernet0 -n 11
EEPROM hexdump for port Ethernet0 page 17h = Should be 11h
        Lower page 0h
        00000000 18 52 00 07 00 00 00 00  00 00 00 00 00 00 34 2f |.R............4/|
        00000010 81 3a 00 00 00 00 00 00  00 00 00 00 00 00 00 ff |.:..............|
        00000020 ff ff ff 00 00 01 00 01  02 00 00 00 00 00 00 00 |................|
        00000030 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000040 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000050 00 00 00 00 00 04 11 03  88 01 0b 02 44 11 0f 03 |............D...|
        00000060 44 11 0d 03 22 55 0a 03  11 ff 05 02 11 ff ff 00 |D..."U..........|
        00000070 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
 
        Upper page 0h
        00000080 18 41 4f 49 20 20 20 20  20 20 20 20 20 20 20 20 |.AOI            |
        00000090 20 00 29 26 41 4c 51 41  39 4e 30 33 41 44 4c 41 | .)&ALQA9N03ADLA|
        000000a0 31 35 38 30 31 30 39 39  39 32 33 47 32 35 30 38 |15801099923G2508|
        000000b0 30 41 20 20 20 20 32 33  30 37 30 36 20 20 20 20 |0A    230706    |
        000000c0 20 20 20 20 20 20 20 20  60 20 43 23 00 00 00 00 |        ` C#....|
        000000d0 00 00 00 02 00 00 00 00  00 00 00 00 00 00 db 00 |................|
        000000e0 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        000000f0 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
 
        Upper page 17h = Should be 11h
        00000080 18 41 4f 49 20 20 20 20  20 20 20 20 20 20 20 20 |.AOI            |
        00000090 20 00 29 26 41 4c 51 41  39 4e 30 33 41 44 4c 41 | .)&ALQA9N03ADLA|
        000000a0 31 35 38 30 31 30 39 39  39 32 33 47 32 35 30 38 |15801099923G2508|
        000000b0 30 41 20 20 20 20 32 33  30 37 30 36 20 20 20 20 |0A    230706    |
        000000c0 20 20 20 20 20 20 20 20  60 20 43 23 00 00 00 00 |        ` C#....|
        000000d0 00 00 00 02 00 00 00 00  00 00 00 00 00 00 db 00 |................|
        000000e0 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        000000f0 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|

New command output (if the output of a command-line utility has changed)

EEPROM hexdump for port Ethernet32 page 11h
        Lower page 0h
        00000000 18 40 80 07 00 00 00 00  00 00 00 00 00 00 00 00 |.@..............|
        00000010 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000020 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000030 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000040 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000050 00 00 00 00 00 03 1d 01  88 01 1c 01 44 11 1b 01 |............D...|
        00000060 22 55 1a 01 44 11 18 01  11 ff 17 01 44 11 16 01 |"U..D.......D...|
        00000070 11 ff 01 01 11 ff 00 00  00 00 00 00 00 00 00 00 |................|

        Upper page 0h
        00000080 18 4d 65 6c 6c 61 6e 6f  78 20 20 20 20 20 20 20 |.Mellanox       |
        00000090 20 00 02 c9 4d 43 50 31  36 36 30 2d 57 30 30 41 | ...MCP1660-W00A|
        000000a0 45 33 30 20 41 34 4d 54  32 32 35 31 56 53 30 32 |E30 A4MT2251VS02|
        000000b0 34 33 38 20 20 20 32 32  31 32 31 36 20 20 20 20 |438   221216    |
        000000c0 20 20 20 20 20 20 20 20  00 01 05 23 04 05 07 15 |        ...#....|
        000000d0 00 00 00 02 0a 00 00 00  00 00 00 00 00 00 af 00 |................|
        000000e0 33 30 33 33 30 4b 51 36  54 33 55 37 00 00 00 00 |30330KQ6T3U7....|
        000000f0 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|

        Upper page 11h
        00000080 18 4d 65 6c 6c 61 6e 6f  78 20 20 20 20 20 20 20 |.Mellanox       |
        00000090 20 00 02 c9 4d 43 50 31  36 36 30 2d 57 30 30 41 | ...MCP1660-W00A|
        000000a0 45 33 30 20 41 34 4d 54  32 32 35 31 56 53 30 32 |E30 A4MT2251VS02|
        000000b0 34 33 38 20 20 20 32 32  31 32 31 36 20 20 20 20 |438   221216    |
        000000c0 20 20 20 20 20 20 20 20  00 01 05 23 04 05 07 15 |        ...#....|
        000000d0 00 00 00 02 0a 00 00 00  00 00 00 00 00 00 af 00 |................|
        000000e0 33 30 33 33 30 4b 51 36  54 33 55 37 00 00 00 00 |30330KQ6T3U7....|
        000000f0 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|

@yuazhe yuazhe marked this pull request as ready for review April 17, 2024 03:29
mihirpat1
mihirpat1 reviewed Apr 17, 2024
View reviewed changes
Copy link
Contributor

@mihirpat1 mihirpat1 left a comment

Choose a reason for hiding this comment

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

@yuazhe Can you please help in adding a unit-test to ensure that the output of page 10h and page 11h are in line with expectation?

prgeor
prgeor previously approved these changes Apr 17, 2024
View reviewed changes
@yuazhe
Fix double hex to decimal conversion
45bf4d3 
In the previous commit with hash a3cf5c that aimed to address the issue
where sfputil incorrectly interpreted page numbers as decimal instead of
hexadecimal, there was an inadvertent double conversion from hexadecimal
to decimal. For instance, inputting 11 resulted in conversion to 17 and
then further to 23. To rectify this, the second conversion would be
removed.

A related ut has also been added.

Signed-off-by: Yuanzhe, Liu <[email protected]>
@prgeor prgeor added the Bug label Apr 19, 2024
@yxieca yxieca merged commit eb165f3 into sonic-net:master Apr 19, 2024
5 checks passed
@mihirpat1
Copy link
Contributor

@yxieca Can you please help in merging this to 202311 branch?
MSFT ADO - 28384811

arfeigin pushed a commit to arfeigin/sonic-utilities that referenced this pull request Jun 16, 2024
@yuazhe @arfeigin
Fix double hex to decimal conversion (sonic-net#3267)
b22581c 
In the previous commit with hash a3cf5c that aimed to address the issue
where sfputil incorrectly interpreted page numbers as decimal instead of
hexadecimal, there was an inadvertent double conversion from hexadecimal
to decimal. For instance, inputting 11 resulted in conversion to 17 and
then further to 23. To rectify this, the second conversion would be
removed.

A related ut has also been added.

Signed-off-by: Yuanzhe, Liu <[email protected]>
@mihirpat1
Copy link
Contributor

@StormLiangMS I have validated this PR on 202311 now. Can you please help in merging this?

mssonicbld pushed a commit to mssonicbld/sonic-utilities that referenced this pull request Jun 20, 2024
@yuazhe @mssonicbld
Fix double hex to decimal conversion (sonic-net#3267)
25fdc3f 
In the previous commit with hash a3cf5c that aimed to address the issue
where sfputil incorrectly interpreted page numbers as decimal instead of
hexadecimal, there was an inadvertent double conversion from hexadecimal
to decimal. For instance, inputting 11 resulted in conversion to 17 and
then further to 23. To rectify this, the second conversion would be
removed.

A related ut has also been added.

Signed-off-by: Yuanzhe, Liu <[email protected]>
@mssonicbld mssonicbld mentioned this pull request Jun 20, 2024
Merged
@yxieca
Copy link
Contributor

yxieca commented Jun 20, 2024

@bingwang-ms this change is not in 202405 branch yet, right?

@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202311: #3379

mssonicbld pushed a commit that referenced this pull request Jun 20, 2024
@yuazhe @mssonicbld
Fix double hex to decimal conversion (#3267)
2d7c6cd 
In the previous commit with hash a3cf5c that aimed to address the issue
where sfputil incorrectly interpreted page numbers as decimal instead of
hexadecimal, there was an inadvertent double conversion from hexadecimal
to decimal. For instance, inputting 11 resulted in conversion to 17 and
then further to 23. To rectify this, the second conversion would be
removed.

A related ut has also been added.

Signed-off-by: Yuanzhe, Liu <[email protected]>
@bingwang-ms
Copy link
Contributor

PR is already in 202405 branch. Removing the label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihirpat1 mihirpat1 mihirpat1 approved these changes

@Junchao-Mellanox Junchao-Mellanox Junchao-Mellanox approved these changes

@prgeor prgeor prgeor approved these changes

Assignees
No one assigned
Labels
Approved for 202311 Branch Bug Cherry Pick Conflict_202405 Included in 202311 Branch Request for 202311 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@yuazhe @mihirpat1 @yxieca @mssonicbld @bingwang-ms @prgeor @Junchao-Mellanox @keboliu