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

Use smart pointer to manage pcfstat object #2177

Merged
merged 1 commit into from
Jul 19, 2023
Merged

Conversation

eryugey
Copy link
Contributor

@eryugey eryugey commented May 31, 2023

Previously pcfstat points to a raw pointer, and it may be leaked if
function returned before deleting it.

So use smart pointer to automatically release the object.

Note that currently s3fs only uses c++03, so we use auto_ptr here, not
unique_ptr, which requires c++11.

Fixes: 6ca5a24 ("Fix two inconsistency issues between stat cache and cache file (#2152)")
Signed-off-by: Eryu Guan eguan@linux.alibaba.com

@gaul
Copy link
Member

gaul commented May 31, 2023

This requires C++11 and s3fs currently only requires C++03. We haven't really talked about this before since C++11's benefits are modest but #1068 has discussion about a library we would like to use which requires C++14.

Copy link
Member

@gaul gaul left a comment

Choose a reason for hiding this comment

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

We are definitely leaking on error paths, probably in other parts of the code as well. I don't have a strong opposition to this but let's discuss it in the related issue.

src/fdcache_entity.cpp Outdated Show resolved Hide resolved
@gaul gaul mentioned this pull request May 31, 2023
@gaul
Copy link
Member

gaul commented May 31, 2023

Alternatively we could use a unique_ptr reimplementation that I experimented with in https://github.com/gaul/s3fs-fuse/pull/new/c++/unique_ptr. However these usually have bugs.

@eryugey eryugey force-pushed the dev branch 2 times, most recently from ba51c80 to 66b0347 Compare June 1, 2023 03:53
@eryugey eryugey changed the title Use unique_ptr to manage pcfstat object Use smart pointer to manage pcfstat object Jun 1, 2023
@eryugey
Copy link
Contributor Author

eryugey commented Jun 1, 2023

We are definitely leaking on error paths, probably in other parts of the code as well. I don't have a strong opposition to this but let's discuss it in the related issue.

OK, I switched to auto_ptr this time, and we could switch it back to unique_ptr once s3fs adopts c++11 or above.

@eryugey eryugey requested a review from gaul June 1, 2023 03:55
Previously pcfstat points to a raw pointer, and it may be leaked if
function returned before deleting it.

So use smart pointer to automatically release the object.

Note that currently s3fs only uses c++03, so we use auto_ptr here, not
unique_ptr, which requires c++11.

Fixes: 6ca5a24 ("Fix two inconsistency issues between stat cache and cache file (s3fs-fuse#2152)")
Signed-off-by: Eryu Guan <eguan@linux.alibaba.com>
Copy link
Member

@ggtakec ggtakec left a comment

Choose a reason for hiding this comment

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

@eryugey Thanks for the fix code.
(This code was recently modified and created many leaks.)

@gaul Please check.
I think we can merge this.

@gaul
Copy link
Member

gaul commented Jun 24, 2023

Discussing this in #2195.

@ggtakec
Copy link
Member

ggtakec commented Jun 25, 2023

@gaul Would you like to close this PR and contain this to #2195?

@gaul gaul merged commit 7978395 into s3fs-fuse:master Jul 19, 2023
13 checks passed
@gaul
Copy link
Member

gaul commented Jul 19, 2023

#2195 is a better approach but let's wait until we sort out 1.93.

gaul added a commit to gaul/s3fs-fuse that referenced this pull request Jul 19, 2023
This prevents a std::auto_ptr warning about not using std::unique_ptr.
Remove similar setting from CI configuration.  References s3fs-fuse#2177.
@ggtakec
Copy link
Member

ggtakec commented Jul 19, 2023

@gaul I agree with your opinion.

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