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

sysx/xattr: fix and improve #150

Merged
merged 2 commits into from Feb 28, 2020
Merged

sysx/xattr: fix and improve #150

merged 2 commits into from Feb 28, 2020

Conversation

kolyshkin
Copy link
Contributor

1. fix getxattrAll

The current implementation is suboptimal for two reasons:

  • the initial buffer size of 5 bytes makes no sense
    (my guess @stevvooe forgot to change it back to a sane
    value after the initial retry logic testing);

  • it never gets the real buffer size, resulting in extra
    iterations (5 -> 10 -> 20 -> ...).

This commit

  • sets the initial size to 128;
  • uses the logic to get the real size in case we get ERANGE
    (rather than doubling the buffer)

2. improve listxattrAll

The current code

  • is hard to read (at least for me, that is);
  • is suboptimal (always calls listxattr() twice);
  • doesn't handle the condition when attributes size
    is changed in between the two calls to listxattr().

Fix all three issues at once by reusing the logic from getxattrAll().

The current implementation is suboptimal for two reasons:

 * the initial buffer size of 5 bytes makes no sense
   (my guess someone forgot to change it back to a sane
   value after the retry logic testing);

 * it never gets the real buffer size, resulting in extra
   iterations (5 -> 10 -> 20 -> ...).

This commit
 * sets the initial size to 128;
 * uses the logic to get the real size in case we get ERANGE

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The current code
 * is hard to read (at least for me, that is);
 * is suboptimal (always calls listxattr() twice);
 * doesn't handle the condition when attributes size
   is changed in between the two calls to listxattr().

Fix all three issues by reusing the logic from getxattrAll().

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Copy link
Member

@Zyqsempai Zyqsempai left a comment

Choose a reason for hiding this comment

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

I like it, but please sign your commits.

@kolyshkin
Copy link
Contributor Author

I like it, but please sign your commits.

@Zyqsempai Sorry, I don't get it. Do you mean something other than the

Signed-off-by: Kir Kolyshkin kolyshkin@gmail.com

line which is present in both commits?

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 0f16d7a into containerd:master Feb 28, 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.

None yet

4 participants