Skip to content

Commit

Permalink
Relanding "Abstrating file access operation in zip creation."
Browse files Browse the repository at this point in the history 
Disabled the added conditions that caused the tests to fail (only on the
build bots) and added logs for investigation.

Bug: 772815
Tbr: isherman
Change-Id: I1f20bfe40288b6822397d469863deb643a93b17d
Reviewed-on: https://chromium-review.googlesource.com/716597
Commit-Queue: Jay Civelli <jcivelli@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508539}
  • Loading branch information
@jcivelli-google
jcivelli-google authored and Commit Bot committed Oct 12, 2017
1 parent f8bd3bd commit 513d84a
Show file tree
Hide file tree
Showing 5 changed files with 438 additions and 115 deletions.
221 changes: 156 additions & 65 deletions third_party/zlib/google/zip.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "third_party/zlib/google/zip.h"

#include <list>
#include <string>
#include <vector>

Expand All @@ -25,10 +26,13 @@
#include "third_party/zlib/contrib/minizip/zip.h"
#endif

namespace zip {
namespace {

bool AddFileToZip(zipFile zip_file, const base::FilePath& src_dir) {
base::File file(src_dir, base::File::FLAG_OPEN | base::File::FLAG_READ);
bool AddFileToZip(zipFile zip_file,
const base::FilePath& src_dir,
FileAccessor* file_accessor) {
base::File file = file_accessor->OpenFileForReading(src_dir);
if (!file.IsValid()) {
DLOG(ERROR) << "Could not open file for path " << src_dir.value();
return false;
Expand All @@ -50,8 +54,10 @@ bool AddFileToZip(zipFile zip_file, const base::FilePath& src_dir) {
return true;
}

bool AddEntryToZip(zipFile zip_file, const base::FilePath& path,
const base::FilePath& root_path) {
bool AddEntryToZip(zipFile zip_file,
const base::FilePath& path,
const base::FilePath& root_path,
FileAccessor* file_accessor) {
base::FilePath relative_path;
bool result = root_path.AppendRelativePath(path, &relative_path);
DCHECK(result);
Expand All @@ -60,17 +66,17 @@ bool AddEntryToZip(zipFile zip_file, const base::FilePath& path,
base::ReplaceSubstringsAfterOffset(&str_path, 0u, "\\", "/");
#endif

bool is_directory = base::DirectoryExists(path);
bool is_directory = file_accessor->DirectoryExists(path);
if (is_directory)
str_path += "/";

zip_fileinfo file_info = zip::internal::GetFileInfoForZipping(path);
if (!zip::internal::ZipOpenNewFileInZip(zip_file, str_path, &file_info))
if (!zip::internal::ZipOpenNewFileInZip(
zip_file, str_path, file_accessor->GetLastModifiedTime(path)))
return false;

bool success = true;
if (!is_directory) {
success = AddFileToZip(zip_file, path);
success = AddFileToZip(zip_file, path, file_accessor);
}

if (ZIP_OK != zipCloseFileInZip(zip_file)) {
Expand All @@ -81,17 +87,151 @@ bool AddEntryToZip(zipFile zip_file, const base::FilePath& path,
return success;
}

bool IsHiddenFile(const base::FilePath& file_path) {
return file_path.BaseName().value()[0] == '.';
}

bool ExcludeNoFilesFilter(const base::FilePath& file_path) {
return true;
}

bool ExcludeHiddenFilesFilter(const base::FilePath& file_path) {
return file_path.BaseName().value()[0] != '.';
return !IsHiddenFile(file_path);
}

class DirectFileAccessor : public FileAccessor {
public:
~DirectFileAccessor() override = default;

base::File OpenFileForReading(const base::FilePath& file) override {
return base::File(file, base::File::FLAG_OPEN | base::File::FLAG_READ);
}

bool DirectoryExists(const base::FilePath& file) override {
return base::DirectoryExists(file);
}

std::vector<DirectoryContentEntry> ListDirectoryContent(
const base::FilePath& dir) {
std::vector<DirectoryContentEntry> files;
base::FileEnumerator file_enumerator(
dir, false /* recursive */,
base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES);
for (base::FilePath path = file_enumerator.Next(); !path.value().empty();
path = file_enumerator.Next()) {
files.push_back(DirectoryContentEntry(path, base::DirectoryExists(path)));
}
return files;
}

base::Time GetLastModifiedTime(const base::FilePath& path) override {
base::File::Info file_info;
if (!base::GetFileInfo(path, &file_info)) {
LOG(ERROR) << "Failed to retrieve file modification time for "
<< path.value();
}
return file_info.last_modified;
}
};

} // namespace

namespace zip {
ZipParams::ZipParams(const base::FilePath& src_dir,
const base::FilePath& dest_file)
: src_dir_(src_dir),
dest_file_(dest_file),
file_accessor_(new DirectFileAccessor()) {}

#if defined(OS_POSIX)
// Does not take ownership of |fd|.
ZipParams::ZipParams(const base::FilePath& src_dir, int dest_fd)
: src_dir_(src_dir),
dest_fd_(dest_fd),
file_accessor_(new DirectFileAccessor()) {}
#endif

bool Zip(const ZipParams& params) {
DCHECK(params.file_accessor()->DirectoryExists(params.src_dir()));

zipFile zip_file = nullptr;
#if defined(OS_POSIX)
int dest_fd = params.dest_fd();
if (dest_fd != base::kInvalidPlatformFile) {
DCHECK(params.dest_file().empty());
zip_file = internal::OpenFdForZipping(dest_fd, APPEND_STATUS_CREATE);
if (!zip_file) {
DLOG(ERROR) << "Couldn't create ZIP file for FD " << dest_fd;
return false;
}
}
#endif
if (!zip_file) {
const base::FilePath& dest_file = params.dest_file();
DCHECK(!dest_file.empty());
zip_file = internal::OpenForZipping(dest_file.AsUTF8Unsafe(),
APPEND_STATUS_CREATE);
if (!zip_file) {
DLOG(WARNING) << "Couldn't create ZIP file at path " << dest_file;
return false;
}
}

// Using a pointer to avoid copies of a potentially large array.
const std::vector<base::FilePath>* files_to_add = &params.files_to_zip();
std::vector<base::FilePath> all_files;
if (files_to_add->empty()) {
// Include all files from the src_dir (modulo the src_dir itself and
// filtered and hidden files).

files_to_add = &all_files;
// Using a list so we can call push_back while iterating.
std::list<FileAccessor::DirectoryContentEntry> entries;
entries.push_back(FileAccessor::DirectoryContentEntry(
params.src_dir(), true /* is directory*/));
const FilterCallback& filter_callback = params.filter_callback();
for (auto iter = entries.begin(); iter != entries.end(); ++iter) {
const base::FilePath& entry_path = iter->path;
if (iter != entries.begin() && // Don't filter the root dir.
((!params.include_hidden_files() && IsHiddenFile(entry_path)) ||
(filter_callback && !filter_callback.Run(entry_path)))) {
continue;
}

if (iter != entries.begin()) { // Exclude the root dir from the ZIP file.
// Make the path relative for AddEntryToZip.
base::FilePath relative_path;
bool success =
params.src_dir().AppendRelativePath(entry_path, &relative_path);
DCHECK(success);
all_files.push_back(relative_path);
}

if (iter->is_directory) {
std::vector<FileAccessor::DirectoryContentEntry> subentries =
params.file_accessor()->ListDirectoryContent(entry_path);
entries.insert(entries.end(), subentries.begin(), subentries.end());
}
}
}

bool success = true;
for (auto iter = files_to_add->begin(); iter != files_to_add->end(); ++iter) {
const base::FilePath& path = params.src_dir().Append(*iter);
if (!AddEntryToZip(zip_file, path, params.src_dir(),
params.file_accessor())) {
// TODO(hshi): clean up the partial zip file when error occurs.
success = false;
break;
}
}

if (ZIP_OK != zipClose(zip_file, NULL)) {
DLOG(ERROR) << "Error closing zip file " << params.dest_file().value();
return false;
}

return success;
}

bool Unzip(const base::FilePath& src_file, const base::FilePath& dest_dir) {
return UnzipWithFilterCallback(src_file, dest_dir,
Expand Down Expand Up @@ -140,36 +280,9 @@ bool ZipWithFilterCallback(const base::FilePath& src_dir,
const base::FilePath& dest_file,
const FilterCallback& filter_cb) {
DCHECK(base::DirectoryExists(src_dir));

zipFile zip_file = internal::OpenForZipping(dest_file.AsUTF8Unsafe(),
APPEND_STATUS_CREATE);

if (!zip_file) {
DLOG(WARNING) << "couldn't create file " << dest_file.value();
return false;
}

bool success = true;
base::FileEnumerator file_enumerator(src_dir, true /* recursive */,
base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES);
for (base::FilePath path = file_enumerator.Next(); !path.value().empty();
path = file_enumerator.Next()) {
if (!filter_cb.Run(path)) {
continue;
}

if (!AddEntryToZip(zip_file, path, src_dir)) {
success = false;
break;
}
}

if (ZIP_OK != zipClose(zip_file, NULL)) {
DLOG(ERROR) << "Error closing zip file " << dest_file.value();
return false;
}

return success;
ZipParams params(src_dir, dest_file);
params.set_filter_callback(filter_cb);
return Zip(params);
}

bool Zip(const base::FilePath& src_dir, const base::FilePath& dest_file,
Expand All @@ -188,31 +301,9 @@ bool ZipFiles(const base::FilePath& src_dir,
const std::vector<base::FilePath>& src_relative_paths,
int dest_fd) {
DCHECK(base::DirectoryExists(src_dir));
zipFile zip_file = internal::OpenFdForZipping(dest_fd, APPEND_STATUS_CREATE);

if (!zip_file) {
DLOG(ERROR) << "couldn't create file for fd " << dest_fd;
return false;
}

bool success = true;
for (std::vector<base::FilePath>::const_iterator iter =
src_relative_paths.begin();
iter != src_relative_paths.end(); ++iter) {
const base::FilePath& path = src_dir.Append(*iter);
if (!AddEntryToZip(zip_file, path, src_dir)) {
// TODO(hshi): clean up the partial zip file when error occurs.
success = false;
break;
}
}

if (ZIP_OK != zipClose(zip_file, NULL)) {
DLOG(ERROR) << "Error closing zip file for fd " << dest_fd;
success = false;
}

return success;
ZipParams params(src_dir, dest_fd);
params.set_files_to_zip(src_relative_paths);
return Zip(params);
}
#endif // defined(OS_POSIX)

Expand Down
100 changes: 100 additions & 0 deletions third_party/zlib/google/zip.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,110 @@

#include "base/callback.h"
#include "base/files/file_path.h"
#include "base/files/platform_file.h"
#include "base/time/time.h"
#include "build/build_config.h"

namespace base {
class File;
}

namespace zip {

// Abstraction for file access operation required by Zip().
// Can be passed to the ZipParams for providing custom access to the files,
// for example over IPC.
// If none is provided, the files are accessed directly.
class FileAccessor {
public:
virtual ~FileAccessor() = default;

struct DirectoryContentEntry {
DirectoryContentEntry(const base::FilePath& path, bool is_directory)
: path(path), is_directory(is_directory) {}
base::FilePath path;
bool is_directory = false;
};

virtual base::File OpenFileForReading(const base::FilePath& path) = 0;
virtual bool DirectoryExists(const base::FilePath& path) = 0;
virtual std::vector<DirectoryContentEntry> ListDirectoryContent(
const base::FilePath& dir_path) = 0;
virtual base::Time GetLastModifiedTime(const base::FilePath& path) = 0;
};

class ZipParams {
public:
ZipParams(const base::FilePath& src_dir, const base::FilePath& dest_file);
#if defined(OS_POSIX)
// Does not take ownership of |dest_fd|.
ZipParams(const base::FilePath& src_dir, int dest_fd);

int dest_fd() const { return dest_fd_; }
#endif

const base::FilePath& src_dir() const { return src_dir_; }

const base::FilePath& dest_file() const { return dest_file_; }

// Restricts the files actually zipped to the paths listed in
// |src_relative_paths|. They must be relative to the |src_dir| passed in the
// constructor and will be used as the file names in the created zip file. All
// source paths must be under |src_dir| in the file system hierarchy.
void set_files_to_zip(const std::vector<base::FilePath>& src_relative_paths) {
src_files_ = src_relative_paths;
}
const std::vector<base::FilePath>& files_to_zip() const { return src_files_; }

using FilterCallback = base::Callback<bool(const base::FilePath&)>;
void set_filter_callback(FilterCallback filter_callback) {
filter_callback_ = filter_callback;
}
const FilterCallback& filter_callback() const { return filter_callback_; }

void set_include_hidden_files(bool include_hidden_files) {
include_hidden_files_ = include_hidden_files;
}
bool include_hidden_files() const { return include_hidden_files_; }

// Sets a custom file accessor for file operations. Default is to directly
// access the files (with fopen and the rest).
// Useful in cases where running in a sandbox process and file access has to
// go through IPC, for example.
void set_file_accessor(std::unique_ptr<FileAccessor> file_accessor) {
file_accessor_ = std::move(file_accessor);
}
FileAccessor* file_accessor() const { return file_accessor_.get(); }

private:
base::FilePath src_dir_;

base::FilePath dest_file_;
#if defined(OS_POSIX)
int dest_fd_ = base::kInvalidPlatformFile;
#endif

// The relative paths to the files that should be included in the zip file. If
// this is empty, all files in |src_dir_| are included.
std::vector<base::FilePath> src_files_;

// Filter used to exclude files from the ZIP file. Only effective when
// |src_files_| is empty.
FilterCallback filter_callback_;

// Whether hidden files should be included in the ZIP file. Only effective
// when |src_files_| is empty.
bool include_hidden_files_ = true;

// Abstraction around file system access used to read files. An implementation
// that accesses files directly is provided by default.
std::unique_ptr<FileAccessor> file_accessor_;
};

// Zip files specified into a ZIP archives. The source files and ZIP destination
// files (as well as other settings) are specified in |params|.
bool Zip(const ZipParams& params);

// Zip the contents of src_dir into dest_file. src_path must be a directory.
// An entry will *not* be created in the zip for the root folder -- children
// of src_dir will be at the root level of the created zip. For each file in
Expand Down
Loading

0 comments on commit 513d84a

Please sign in to comment.