Remove path member from zzip so it can be tested in memory only, and write tests.

Author: akrieger

src/debug_menu.cpp

@@ -484,8 +484,7 @@ void write_min_archive()
                 {
                     std::optional<zzip> min_mmr_temp_zzip = zzip::load( min_mmr_temp_zzip_path, mmr_dict );
                     if( !min_mmr_temp_zzip ||
-                        !mmr_stack->copy_files_to( trimmed_mmr_entries, *min_mmr_temp_zzip ) ||
-                        !min_mmr_temp_zzip->compact( 0 ) ) {
+                        !mmr_stack->copy_files_to( trimmed_mmr_entries, *min_mmr_temp_zzip ) ) {
                         popup( _( "Failed to create minimized archive" ) );
                         return;
                     }

src/game.cpp

@@ -3500,7 +3500,8 @@ bool game::save_player_data()
                                               ".zzip.tmp" ).get_unrelative_path() );
         saved_data = z->add_file( ( playerfile + SAVE_EXTENSION ).get_unrelative_path().filename(),
                                   save.str() );
-        saved_data = saved_data && z->compact( 1.0 );
+        saved_data = saved_data && z->compact_to( ( playerfile + SAVE_EXTENSION +
+                     ".zzip" ).get_unrelative_path(), 0.0 );
     } else {
         saved_data = write_to_file( playerfile + SAVE_EXTENSION, [&]( std::ostream & fout ) {
             serialize_json( fout );

src/mapbuffer.cpp

@@ -254,12 +254,12 @@ void mapbuffer::save_quad(
     bool file_exists = false;
 
     std::optional<zzip> z;
+    cata_path zzip_name = dirname;
+    zzip_name += ".zzip";
     // The number of uniform submaps is so enormous that the filesystem overhead
     // for this step of just checking if the quad exists approaches 70% of the
     // total cost of saving the mapbuffer, in one test save I had.
     if( world_generator->active_world->has_compression_enabled() ) {
-        cata_path zzip_name = dirname;
-        zzip_name += ".zzip";
         z = zzip::load( zzip_name.get_unrelative_path(),
                         ( PATH_INFO::world_base_save_path() / "maps.dict" ).get_unrelative_path() );
         if( !z ) {
@@ -342,7 +342,6 @@ void mapbuffer::save_quad(
 
     if( z ) {
         z->add_file( filename.get_relative_path().filename(), s );
-        z->compact( 2.0 );
     } else {
         // Don't create the directory if it would be empty
         assure_dir_exist( dirname );
@@ -358,6 +357,13 @@ void mapbuffer::save_quad(
             std::filesystem::remove( filename.get_unrelative_path() );
         }
     }
+    if( z ) {
+        cata_path tmp_path = zzip_name + ".tmp";
+        if( z->compact_to( tmp_path.get_unrelative_path(), 2.0 ) ) {
+            z.reset();
+            rename_file( tmp_path, zzip_name );
+        }
+    }
 }
 
 // We're reading in way too many entities here to mess around with creating sub-objects and

src/overmap.cpp

@@ -6758,7 +6758,11 @@ void overmap::save() const
             throw std::runtime_error( string_format( "Failed to save omap %d.%d to %s", loc.x(),
                                       loc.y(), zzip_path.get_unrelative_path().generic_u8string().c_str() ) );
         }
-        z->compact( 2.0 );
+        cata_path tmp_path = zzip_path + ".tmp";
+        if( z->compact_to( tmp_path.get_unrelative_path(), 2.0 ) ) {
+            z.reset();
+            rename_file( tmp_path, zzip_path );
+        }
     } else {
         write_to_file( PATH_INFO::world_base_save_path() / overmapbuffer::terrain_filename( loc ), [&](
         std::ostream & stream ) {

src/zzip.cpp

@@ -4,7 +4,6 @@
 #include <array>
 #include <cstring>
 #include <exception>
-#include <functional>
 #include <iosfwd>
 #include <memory>
 #include <optional>
@@ -23,7 +22,6 @@
 #include <zstd/common/mem.h>
 #include <zstd/common/xxhash.h>
 
-#include "cata_scope_helpers.h"
 #include "filesystem.h"
 #include "flexbuffer_cache.h"
 #include "flexbuffer_json.h"
@@ -339,9 +337,8 @@ struct zzip::context {
     ZSTD_DCtx *dctx;
 };
 
-zzip::zzip( std::filesystem::path path, std::shared_ptr<mmap_file> file, JsonObject footer )
-    : path_{ std::move( path ) },
-      file_{ std::move( file ) },
+zzip::zzip( std::shared_ptr<mmap_file> file, JsonObject footer )
+    : file_{ std::move( file ) },
       footer_{ std::move( footer ) }
 {}
 
@@ -358,6 +355,12 @@ std::optional<zzip> zzip::load( std::filesystem::path const &path,
 {
     std::shared_ptr<mmap_file> file = mmap_file::map_writeable_file( path );
 
+    return load( std::move( file ), dictionary_path );
+}
+std::optional<zzip> zzip::load(
+    std::shared_ptr<mmap_file> file,
+    std::filesystem::path const &dictionary_path )
+{
     flexbuffers::Reference root;
     std::shared_ptr<parsed_flexbuffer> flexbuffer;
     JsonObject footer;
@@ -386,7 +389,7 @@ std::optional<zzip> zzip::load( std::filesystem::path const &path,
         }
     }
 
-    std::optional<zzip> ret{ std::in_place, zzip{path, std::move( file ), std::move( footer )} };
+    std::optional<zzip> ret{ std::in_place, zzip{std::move( file ), std::move( footer )} };
     zzip &zip = ret.value();
 
     ZSTD_CCtx *cctx;
@@ -471,7 +474,7 @@ bool zzip::add_file( std::filesystem::path const &zzip_relative_path, std::strin
 
 
 bool zzip::copy_files( std::vector<std::filesystem::path> const &zzip_relative_paths,
-                       zzip const &from )
+                       zzip const &from, bool shrink_to_fit )
 {
     if( zzip_relative_paths.empty() ) {
         return true;
@@ -505,7 +508,7 @@ bool zzip::copy_files( std::vector<std::filesystem::path> const &zzip_relative_p
         entry_to_copy.offset = new_content_end;
         new_content_end += entry_to_copy.len;
     }
-    return update_footer( original_footer, new_content_end, entries_to_copy );
+    return update_footer( original_footer, new_content_end, entries_to_copy, shrink_to_fit );
 }
 
 
@@ -602,11 +605,6 @@ size_t zzip::get_zzip_size() const
     return file_->len();
 }
 
-std::filesystem::path const &zzip::get_path() const
-{
-    return path_;
-}
-
 std::vector<std::filesystem::path> zzip::get_entries() const
 {
     zzip_footer footer{ footer_ };
@@ -650,14 +648,23 @@ JsonObject zzip::copy_footer() const
     }
 }
 
+namespace
+{
+size_t round_up_file_size( size_t bytes )
+{
+    // Round up to a whole page. Drives nowadays are all 4k pages.
+    size_t needed_pages = std::max( size_t{ 1 }, ( bytes + kAssumedPageSize - 1 ) / kAssumedPageSize );
+
+    return needed_pages * kAssumedPageSize;
+}
+}
+
 // Ensures the zzip file has room to write at least this many bytes.
 // Returns the guaranteed size of the file, which is at least bytes large.
 size_t zzip::ensure_capacity_for( size_t bytes )
 {
     // Round up to a whole page. Drives nowadays are all 4k pages.
-    size_t needed_pages = std::max( size_t{1}, ( bytes + kAssumedPageSize - 1 ) / kAssumedPageSize );
-
-    size_t new_size = needed_pages * kAssumedPageSize;
+    size_t new_size = round_up_file_size( bytes );
 
     if( file_->len() < new_size && !file_->resize_file( new_size ) ) {
         return 0;
@@ -730,7 +737,8 @@ size_t zzip::write_file_at( std::string_view filename, std::string_view content,
 // original JsonObject and inserting the given new entries.
 // If shrink_to_fit is true, will shrink the file as needed to eliminate padding bytes
 // between the content and the footer.
-bool zzip::update_footer( JsonObject const &original_footer, size_t content_end,
+bool zzip::update_footer( JsonObject const &original_footer,
+                          size_t content_end,
                           const std::vector<compressed_entry> &entries, bool shrink_to_fit )
 {
     std::unordered_set<std::string> processed_files;
@@ -779,12 +787,12 @@ bool zzip::update_footer( JsonObject const &original_footer, size_t content_end,
     auto buf = builder.GetBuffer();
 
     if( shrink_to_fit ) {
-        return false;
         if( !file_->resize_file( content_end + buf.size() ) ) {
+            return false;
         }
     } else {
-        size_t guaranteed_capacity = ensure_capacity_for( content_end + buf.size() );
-        if( guaranteed_capacity == 0 ) {
+        size_t guaranteed_capacity = round_up_file_size( content_end + buf.size() );
+        if( guaranteed_capacity == 0 || !file_->resize_file( guaranteed_capacity ) ) {
             return false;
         }
     }
@@ -803,13 +811,13 @@ bool zzip::update_footer( JsonObject const &original_footer, size_t content_end,
     std::shared_ptr<parsed_flexbuffer> new_flexbuffer = std::make_shared<zzip_flexbuffer>(
                 std::move( new_storage )
             );
-    footer_ = JsonValue( std::move( new_flexbuffer ), new_root, nullptr, 0 );
 
+    footer_ = JsonValue( std::move( new_flexbuffer ), new_root, nullptr, 0 );
     return true;
 }
 
 // Scans the zzip to read what data we can validate and write a fresh footer.
-bool zzip::rewrite_footer()
+bool zzip::rewrite_footer( bool shrink_to_fit )
 {
     const size_t zzip_len = file_->len();
     size_t scan_offset = 0;
@@ -910,7 +918,7 @@ bool zzip::rewrite_footer()
         entries.emplace_back( std::move( entry ) );
     }
 
-    return update_footer( JsonObject{}, scan_offset, entries, /* shrink_to_fit = */ true );
+    return update_footer( JsonObject{}, scan_offset, entries, shrink_to_fit );
 }
 
 std::optional<zzip> zzip::create_from_folder( std::filesystem::path const &path,
@@ -1031,12 +1039,17 @@ bool zzip::extract_to_folder( std::filesystem::path const &path,
         return false;
     }
 
-    for( const JsonMember &entry : zip->footer_.get_object( kEntriesKey ) ) {
+    return zip->extract_to_folder( folder );
+}
+
+bool zzip::extract_to_folder( std::filesystem::path const &folder )
+{
+    for( const JsonMember &entry : footer_.get_object( kEntriesKey ) ) {
         if( entry.name().empty() ) {
             continue;
         }
         std::filesystem::path filename = std::filesystem::u8path( entry.name() );
-        size_t len = zip->get_file_size( filename );
+        size_t len = get_file_size( filename );
         std::filesystem::path destination_file_path = folder / filename;
         if( !assure_dir_exist( destination_file_path.parent_path() ) ) {
             return false;
@@ -1045,7 +1058,7 @@ bool zzip::extract_to_folder( std::filesystem::path const &path,
         if( !file || !file->resize_file( len ) ) {
             return false;
         }
-        if( zip->get_file_to( filename, static_cast<std::byte *>( file->base() ), file->len() ) != len ) {
+        if( get_file_to( filename, static_cast<std::byte *>( file->base() ), file->len() ) != len ) {
             return false;
         }
     }
@@ -1111,7 +1124,7 @@ bool zzip::delete_files( std::unordered_set<std::filesystem::path, std_fs_path_h
     return !errored;
 }
 
-bool zzip::compact( double bloat_factor )
+bool zzip::compact_to( std::filesystem::path dest, double bloat_factor )
 {
     zzip_footer footer{ footer_ };
     std::optional<zzip_meta> meta_opt = footer.get_meta();
@@ -1135,69 +1148,26 @@ bool zzip::compact( double bloat_factor )
         }
     }
 
-    std::vector<compressed_entry> compressed_entries = footer.get_entries();
-
-    // If we were compacting in place, sorting left to right would ensure that we
-    // only move entries left that we need to move. Unchanged data would stay on the left.
-    // However that exposes us to data loss on power loss because zzip recovery
-    // relies on an intact sequence of zstd frames.
-    std::sort( compressed_entries.begin(), compressed_entries.end(), []( const compressed_entry & lhs,
-    const compressed_entry & rhs ) {
-        return lhs.offset < rhs.offset;
-    } );
-
-    std::filesystem::path tmp_path = path_;
-    tmp_path.replace_extension( ".zzip.tmp" ); // NOLINT(cata-u8-path)
-
-    std::shared_ptr<mmap_file> compacted_file = mmap_file::map_writeable_file( tmp_path );
-    if( !compacted_file ||
-        !compacted_file->resize_file( meta.total_content_size + kFixedSizeOverhead ) ) {
+    std::unique_ptr<mmap_file> compacted_file = mmap_file::map_writeable_file( dest );
+    if( !compacted_file ) {
         return false;
     }
 
-    size_t current_end = kFooterChecksumFrameSize;
-    for( compressed_entry &entry : compressed_entries ) {
-        memcpy( static_cast<char *>( compacted_file->base() ) + current_end,
-                file_base_plus( entry.offset ),
-                entry.len
-              );
-        entry.offset = current_end;
-        current_end += entry.len;
-    }
-
-    // We can swap the old mmap'd file with the new one
-    // and write a fresh footer directly into it.
-    JsonObject old_footer{ copy_footer() };
-    old_footer.allow_omitted_members();
-
-    file_ = compacted_file;
-    compacted_file = nullptr;
-
-    std::error_code ec;
-    on_out_of_scope reset_on_failure{ [&] {
-            file_ = mmap_file::map_writeable_file( path_ );
-            std::filesystem::remove( tmp_path, ec );
-        } };
-
-    if( !update_footer( old_footer, current_end, compressed_entries, /* shrink_to_fit = */ true ) ) {
-        return false;
-    }
+    return compact_to( std::move( compacted_file ) );
+}
 
-    // Swap the tmp file over the real path. Give it a couple tries in case of filesystem races.
-    size_t attempts = 0;
-    do {
-        std::filesystem::rename( tmp_path, path_, ec );
-    } while( ec && ++attempts < 3 );
-    if( ec ) {
+bool zzip::compact_to( std::shared_ptr<mmap_file> dest )
+{
+    std::optional<zzip> new_zip = zzip::load( dest );
+    if( !new_zip ) {
         return false;
     }
-    reset_on_failure.cancel();
-    return true;
+    return new_zip->copy_files( get_entries(), *this, /* shrink_to_fit = */ true );
 }
 
 bool zzip::clear()
 {
-    return file_->resize_file( 0 ) && rewrite_footer();
+    return file_->resize_file( 0 ) && rewrite_footer( /* shrink_to_fit = */ true );
 }
 
 // Can't directly increment void*, have to cast to char* first.