Skip to content

Conversation

@kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Sep 29, 2020

It might be a tad slower but it surely more correct and well maintained,
so it's better to use it than rely on a custom implementation which is
kind of hard to get entirely right.

This (together with / after) #2599 removes the last custom mountinfo
parser from the codebase (we had five of those).

It might be a tad slower but it surely more correct and well maintained,
so it's better to use it than rely on a custom implementation which is
kind of hard to get entirely right.

Signed-off-by: Kir Kolyshkin <[email protected]>
@AkihiroSuda
Copy link
Member

@Creatone @xiaochenshen PTAL

fields := strings.Split(text, " ")
// Safe as mountinfo encodes mountpoints with spaces as \040.
index := strings.Index(text, " - ")
postSeparatorFields := strings.Fields(text[index+3:])

Choose a reason for hiding this comment

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

I was working with this code because there was a possibility of accessing elements that not exist and that could cause a panic error.
Thanks, @kolyshkin for fixing it and improve code readability.

I adjusted the unit tests to your code. Could you consider using them?

kolyshkin/runc@intel-rdt...Creatone:creatone/intelrdt-moby-sys-mountinfo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left some comments then decided it's easier to rewrite the code. Hope it's OK with you.

Choose a reason for hiding this comment

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

I'm fine with this 👍

Heavily based on work by Paweł Szulik <[email protected]>

Signed-off-by: Kir Kolyshkin <[email protected]>
@Creatone
Copy link

LGTM

Copy link
Contributor

@xiaochenshen xiaochenshen left a comment

Choose a reason for hiding this comment

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

LGTM.

I have run some mount/umount test cases today. It works well and no functional change found.
Thank you @kolyshkin @Creatone for fixing and enhancement for Intel RDT.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@mrunalp mrunalp merged commit ffc30bb into opencontainers:master Sep 30, 2020
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.

6 participants