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.

Sign up for GitHub

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

Jump to bottom

Qt: Fix saved log filename #14239

Merged
merged 3 commits into from Jul 21, 2023
Merged

Qt: Fix saved log filename #14239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b3c9777
Qt: Fix saved log filename
elad335 Jul 21, 2023
2bd0e72
File.h: Implement fs::error::xdev
elad335 Jul 21, 2023
8f90b15
Fix log data loss on force application shutdown
elad335 Jul 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions Utilities/File.cpp
View file
Open in desktop
Expand Up @@ -130,6 +130,7 @@ static fs::error to_error(DWORD e)
case ERROR_NOT_READY: return fs::error::noent;
case ERROR_FILENAME_EXCED_RANGE: return fs::error::toolong;
case ERROR_DISK_FULL: return fs::error::nospace;
case ERROR_NOT_SAME_DEVICE: return fs::error::xdev;
default: return fs::error::unknown;
}
}
Expand Down Expand Up @@ -172,6 +173,7 @@ static fs::error to_error(int e)
case EROFS: return fs::error::readonly;
case EISDIR: return fs::error::isdir;
case ENOSPC: return fs::error::nospace;
case EXDEV: return fs::error::xdev;
default: return fs::error::unknown;
}
}
Expand Down Expand Up @@ -2360,6 +2362,7 @@ void fmt_class_string<fs::error>::format(std::string& out, u64 arg)
case fs::error::isdir: return "Is a directory";
case fs::error::toolong: return "Path too long";
case fs::error::nospace: return "Not enough space on the device";
case fs::error::xdev: return "Device mismatch";
case fs::error::unknown: return "Unknown system error";
}

Expand Down
1 change: 1 addition & 0 deletions Utilities/File.h
View file
Open in desktop
Expand Up @@ -704,6 +704,7 @@ namespace fs
isdir,
toolong,
nospace,
xdev,
unknown
};

Expand Down
19 changes: 16 additions & 3 deletions rpcs3/rpcs3qt/main_window.cpp
View file
Open in desktop
Expand Up @@ -2376,9 +2376,14 @@ void main_window::CreateConnects()
}

// Get new filename from title and title ID but simplified
std::string log_filename = Emu.GetTitleID().empty() ? "RPCS3" : Emu.GetTitleAndTitleID();
log_filename.erase(std::remove_if(log_filename.begin(), log_filename.end(), [](u8 c){ return !std::isalnum(c) && c != ' ' && c != '[' && ']'; }), log_filename.end());
fmt::trim_back(log_filename);
QString log_filename_q = qstr(Emu.GetTitleID().empty() ? "RPCS3" : Emu.GetTitleAndTitleID());
ensure(!log_filename_q.isEmpty());

// Replace unfitting characters
std::replace_if(log_filename_q.begin(), log_filename_q.end(), [](QChar c){ return !c.isLetterOrNumber() && c != QChar::Space && c != '[' && c != ']'; }, QChar::Space);
log_filename_q = log_filename_q.simplified();

const std::string log_filename = log_filename_q.toStdString();

QString path_last_log = m_gui_settings->GetValue(gui::fd_save_log).toString();

Expand Down Expand Up @@ -2408,6 +2413,12 @@ void main_window::CreateConnects()
// Try to copy it if fails
if (fs::copy_file(from, to, true))
{
if (fs::file sync_fd{to, fs::write})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a param of copy_file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for the move operation not the copy, it's outside the scope of copy_file. It may be added for rename in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't make any sense

Copy link
Contributor Author

@elad335 elad335 Jul 21, 2023 •
edited

Choose a reason for hiding this comment

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

Remove file operation may be ordered before file write. On powerloss, this can result in a loss of log file data.

Copy link
Contributor

Choose a reason for hiding this comment

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

This still doesn't explain why you didn't make it an option for copy_file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because syncronizing disk file writes is an expensive operation and has very specific use cases. It's better for it not to be in fs::copy_file nor in fs::file::write to avoid misuse.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's literally the only reason why you would sync.
And that still doesn't explain why it's not a parameter

Copy link
Contributor Author

@elad335 elad335 Jul 21, 2023 •
edited

Choose a reason for hiding this comment

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

It should be used with caution due to the performance panelty, a programmer can easily use it without giving it too much thought which can be detrimental. Boolean parameter can easily slip in review because it is very subtle. Yes we can add a specially typed constant for it to be very explicit such as file.write(buffer, fs::syncronize_this_write_to_disk) but it seems to be overkill.

{
// Prevent data loss (expensive)
sync_fd.sync();
}

fs::remove_file(from);
return true;
}
Expand All @@ -2432,6 +2443,7 @@ void main_window::CreateConnects()
{
m_gui_settings->SetValue(gui::fd_save_log, dir_path);
gui_log.success("Moved log file to '%s'!", dest_archived_path);
gui::utils::open_dir(dest_archived_path);
return;
}

Expand All @@ -2454,6 +2466,7 @@ void main_window::CreateConnects()
{
m_gui_settings->SetValue(gui::fd_save_log, dir_path);
gui_log.success("Moved log file to '%s'!", dest_raw_file_path);
gui::utils::open_dir(dest_raw_file_path);
return;
}

Expand Down