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.

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

fix bug when /proc/net/dev contains unexpected line #421

Merged

Conversation

bo-er
Copy link
Contributor

@bo-er bo-er commented Nov 11, 2021

In centos 8(4.18.0-193.28.1.el8_2.x86_64) if I try this command:

ip link add team0:1 type dummy

I will get a message:

"RTNETLINK answers: Invalid argument". 

However if I do the same thing on Centos 7(3.10.0-862.14.4.el7.x86_64), there is no error ! And If I cat /proc/net/dev I can see there is a sub interface:

Inter-|   Receive                                                |  Transmit
 face |bytes    packets errs drop fifo frame compressed multicast|bytes    packets errs drop fifo colls carrier compressed
  eth0: 2489766199  690315    0    0    0     0          0         0 96022876  713917    0    0    0     0       0          0
    lo: 402582904  916378    0    0    0     0          0         0 402582904  916378    0    0    0     0       0          0
team0:1:       0       0    0    0    0     0          0         0        0       0    0    0    0     0       0          0

So the problem is currently 'procfs' cann't parse /proc/net/dev that contains odd line of interfaces.

Copy link
Member

@discordianfish discordianfish left a comment

Choose a reason for hiding this comment

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

Interesting.. well the actual problem is that this assumes a device name can't include :: https://github.com/prometheus/procfs/pull/421/files#diff-a691799f566ae988549d796fb8df91095344d63552424e79c7687a14dcaaf601R92

But we shouldn't just ignore parse errors. Instead, if we want to fix that, we need to parse this differently.

@discordianfish
Copy link
Member

Thanks! Can you also add test cases for dev names that include :?

@bo-er
Copy link
Contributor Author

bo-er commented Nov 21, 2021

@discordianfish hello! 🙇🏻‍♂️ Do I need to add more test cases?

Copy link
Member

@discordianfish discordianfish left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@discordianfish
Copy link
Member

you need to sign your commit though before we can merge. See the DCO check

bo-er and others added 3 commits November 22, 2021 22:16
Signed-off-by: 吴家榜 <14038@chinasws.com>
Signed-off-by: 吴家榜 <14038@chinasws.com>
Signed-off-by: 吴家榜 <14038@chinasws.com>
@bo-er
Copy link
Contributor Author

bo-er commented Nov 23, 2021

you need to sign your commit though before we can merge. See the DCO check

I have signed my commits.

@SuperQ
Copy link
Member

SuperQ commented Nov 23, 2021

We should probably fix this the same way we fixed it in the node_exporter. I forgot we have yet to convert the node_exporter to this parser.

prometheus/node_exporter#904
prometheus/node_exporter#910

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Actually, I like this better, it avoids use of regexp.

@discordianfish discordianfish merged commit 081c329 into prometheus:master Nov 24, 2021
remijouannet pushed a commit to remijouannet/procfs that referenced this pull request Oct 20, 2022
* fix bug when /proc/net/dev contains unexpected line

Signed-off-by: 吴家榜 <14038@chinasws.com>

* modifying netDev.parseLine due to odd interface name

Signed-off-by: 吴家榜 <14038@chinasws.com>

* adding testing case for net device that has colon in its name

Signed-off-by: 吴家榜 <14038@chinasws.com>

Co-authored-by: wujiabang <wujiabang@actionsky.com>
Co-authored-by: 吴家榜 <14038@chinasws.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants