Skip to content

Commit

Permalink
Revert "Relanding "Abstrating file access operation in zip creation.""
Browse files Browse the repository at this point in the history 
This reverts commit 513d84a.

Reason for revert: it breaks the unit-tests.

Original change's description:
> Relanding "Abstrating file access operation in zip creation."
> 
> 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}

TBR=jcivelli@chromium.org,isherman@chromium.org

Change-Id: Ife1a2ebe47b29b4231125aef4058c489d254baf4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 772815
Reviewed-on: https://chromium-review.googlesource.com/717737
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Commit-Queue: Jay Civelli <jcivelli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508578}
  • Loading branch information
Jay Civelli authored and Commit Bot committed Oct 13, 2017
1 parent 7ae427b commit adb61db
Show file tree
Hide file tree
Showing 5 changed files with 115 additions and 438 deletions.
221 changes: 65 additions & 156 deletions third_party/zlib/google/zip.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

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

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

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

namespace zip {
namespace {

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

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

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

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

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

if (ZIP_OK != zipCloseFileInZip(zip_file)) {
Expand All @@ -87,151 +81,17 @@ bool AddEntryToZip(zipFile zip_file,
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 !IsHiddenFile(file_path);
return file_path.BaseName().value()[0] != '.';
}

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

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;
}
namespace zip {

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

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;
}

bool Zip(const base::FilePath& src_dir, const base::FilePath& dest_file,
Expand All @@ -301,9 +188,31 @@ bool ZipFiles(const base::FilePath& src_dir,
const std::vector<base::FilePath>& src_relative_paths,
int dest_fd) {
DCHECK(base::DirectoryExists(src_dir));
ZipParams params(src_dir, dest_fd);
params.set_files_to_zip(src_relative_paths);
return Zip(params);
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;
}
#endif // defined(OS_POSIX)

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

#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

0 comments on commit adb61db

Please sign in to comment.