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

Flush buffer after forming archive #76

Merged
merged 1 commit into from
Aug 14, 2023
Merged

Flush buffer after forming archive #76

merged 1 commit into from
Aug 14, 2023

Conversation

janaknat
Copy link
Contributor

Data remains in the GzEncoder. Ensure it is flushed before performing any other operations on the tarball.

This fixes a bug in the report generation, where the tarball of the raw data is copied to data/archive/. When trying to untar the file, the data is corrupted and un-useable.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

aperf_report_works.tar.gz
aperf_report_worksnt.tar.gz

src/report.rs Outdated
@@ -227,6 +228,7 @@ pub fn report(report: &Report) -> Result<()> {
let enc = GzEncoder::new(tar_gz, Compression::default());
let mut tar = tar::Builder::new(enc);
tar.append_dir_all(&report_name, &report_name)?;
tar.into_inner()?.flush()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary, tar is going out of scope at the end of this function and so will be flushed as part of that destruction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Was being extra careful so that if, in the future, somebody did something after this, we won't be bit. I've removed it now.

src/report.rs Outdated
@@ -47,6 +47,7 @@ pub fn form_and_copy_archive(loc: String, report_name: &PathBuf) -> Result<()> {
let enc = GzEncoder::new(tar_gz, Compression::default());
let mut tar = tar::Builder::new(enc);
tar.append_dir_all(&dir_stem, &loc)?;
tar.into_inner()?.flush()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Put this in a block so tar gets destroyed now that writing of the tar file is complete. Even this flush isn't 100% safe as you're flushing the GzEncoder, but there could conceivably still be tar-related data to write (stuff written when the tar stream is closed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to being in a block.

src/lib.rs Outdated
@@ -209,6 +210,7 @@ impl PerformanceData {
let enc = GzEncoder::new(tar_gz, Compression::default());
let mut tar = tar::Builder::new(enc);
tar.append_dir_all(&dir_name, &self.init_params.dir_name)?;
tar.into_inner()?.flush()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

tar is going out of scope before the output file could be used, so this is unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Data remains in the GzEncoder. Ensure it is flushed before performing
any other operations on the tarball.

This fixes a bug in the report generation, where the tarball of the raw
data is copied to data/archive/. When trying to untar the file, the data
is corrupted and unuseable.
@janaknat janaknat merged commit fd95803 into main Aug 14, 2023
4 checks passed
@janaknat janaknat deleted the tar-fix branch August 15, 2023 16:13
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.

2 participants