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

common: fix privatefile create. #9270

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0xFFFC0000
Copy link
Collaborator

When creating a private file we need to delete the file if exists.

@selsta
Copy link
Collaborator

selsta commented Apr 15, 2024

@0xFFFC0000 please also open against release branch

@@ -123,6 +123,14 @@ namespace tools

private_file private_file::create(std::string name)
{
if (epee::file_io_utils::is_file_exist(name)) {

Choose a reason for hiding this comment

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

This change seems structurally and semantically quite dubious:

  1. Create tends to be a one-way operation, very different from drop and recreate. If you want to support a "replace" operation, you'd be better off making a new function which wraps the existing and allow the caller here to optionally request O_EXCL.
  2. The existing usage of this function that I see in wallet_rpc_server seems well aware it doesn't remove the existing file; what caller does this help, or contract (documented in the comments or implied by usage) does it honor?

Relevant wallet_rpc_server code:

        rpc_login_file = tools::private_file::create(temp);
        if (!rpc_login_file.handle())
        {
          LOG_ERROR(tr("Failed to create file ") << temp << tr(". Check permissions or remove file"));
          return false;
        }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your comment @iamamyth
Few points:

  1. Apologies for the late reply, I have been extremely busy the past few weeks.
  2. I thought about your comment. And your criticism makes sense to me.

In the new push, I am addressing those issues you have raised.

Copy link

Choose a reason for hiding this comment

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

Looks good; thanks for spotting and fixing the bug.

When creating a private file we need to delete the file if exist.
@0xFFFC0000 0xFFFC0000 force-pushed the dev/0xfffc/fix_private_file_create branch from 2966d83 to 267e31f Compare April 22, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants