From 43a4a8c777fbb8f71540ac7fbe82674489ef557b Mon Sep 17 00:00:00 2001 From: Christopher Tate Date: Thu, 8 Jan 2015 18:42:33 -0800 Subject: Fix redundant file backups We'd observed a bug in which an unchanged file was nevertheless being redundantly transmitted for backup on every backup pass. The underlying issue turns out to have been the FileBackupHelper base implementation's logic for diffing the prior-state file set against the current state, in the case when there had been deletions of prior files. In addition, there was also a parallel bug in which file checksums were not calculated properly in some cases, leading to at least one additional redundant backup of the file in question. Bug 18694053 Change-Id: Ie0dec06486b5fef4624561737019569c85d6b2a0 --- libs/androidfw/BackupHelpers.cpp | 79 +++++++++++++++++++++------------------- 1 file changed, 41 insertions(+), 38 deletions(-) (limited to 'libs/androidfw') diff --git a/libs/androidfw/BackupHelpers.cpp b/libs/androidfw/BackupHelpers.cpp index 52dce9f..33cf8ef 100644 --- a/libs/androidfw/BackupHelpers.cpp +++ b/libs/androidfw/BackupHelpers.cpp @@ -225,8 +225,6 @@ write_update_file(BackupDataWriter* dataStream, int fd, int mode, const String8& file_metadata_v1 metadata; char* buf = (char*)malloc(bufsize); - int crc = crc32(0L, Z_NULL, 0); - fileSize = lseek(fd, 0, SEEK_END); lseek(fd, 0, SEEK_SET); @@ -310,8 +308,12 @@ write_update_file(BackupDataWriter* dataStream, const String8& key, char const* } static int -compute_crc32(int fd) -{ +compute_crc32(const char* file, FileRec* out) { + int fd = open(file, O_RDONLY); + if (fd < 0) { + return -1; + } + const int bufsize = 4*1024; int amt; @@ -324,8 +326,11 @@ compute_crc32(int fd) crc = crc32(crc, (Bytef*)buf, amt); } + close(fd); free(buf); - return crc; + + out->s.crc32 = crc; + return NO_ERROR; } int @@ -353,7 +358,8 @@ back_up_files(int oldSnapshotFD, BackupDataWriter* dataStream, int newSnapshotFD err = stat(file, &st); if (err != 0) { - r.deleted = true; + // not found => treat as deleted + continue; } else { r.deleted = false; r.s.modTime_sec = st.st_mtime; @@ -361,12 +367,17 @@ back_up_files(int oldSnapshotFD, BackupDataWriter* dataStream, int newSnapshotFD //r.s.modTime_nsec = st.st_mtime_nsec; r.s.mode = st.st_mode; r.s.size = st.st_size; - // we compute the crc32 later down below, when we already have the file open. if (newSnapshot.indexOfKey(key) >= 0) { LOGP("back_up_files key already in use '%s'", key.string()); return -1; } + + // compute the CRC + if (compute_crc32(file, &r) != NO_ERROR) { + ALOGW("Unable to open file %s", file); + continue; + } } newSnapshot.add(key, r); } @@ -374,49 +385,41 @@ back_up_files(int oldSnapshotFD, BackupDataWriter* dataStream, int newSnapshotFD int n = 0; int N = oldSnapshot.size(); int m = 0; + int M = newSnapshot.size(); - while (nWriteEntityHeader(p, -1); + write_delete_file(dataStream, p); n++; - } - else if (cmp > 0) { + } else if (cmp > 0) { // file added - LOGP("file added: %s", g.file.string()); + LOGP("file added: %s crc=0x%08x", g.file.string(), g.s.crc32); write_update_file(dataStream, q, g.file.string()); m++; - } - else { - // both files exist, check them + } else { + // same file exists in both old and new; check whether to update const FileState& f = oldSnapshot.valueAt(n); - int fd = open(g.file.string(), O_RDONLY); - if (fd < 0) { - // We can't open the file. Don't report it as a delete either. Let the - // server keep the old version. Maybe they'll be able to deal with it - // on restore. - LOGP("Unable to open file %s - skipping", g.file.string()); - } else { - g.s.crc32 = compute_crc32(fd); - - LOGP("%s", q.string()); - LOGP(" new: modTime=%d,%d mode=%04o size=%-3d crc32=0x%08x", - f.modTime_sec, f.modTime_nsec, f.mode, f.size, f.crc32); - LOGP(" old: modTime=%d,%d mode=%04o size=%-3d crc32=0x%08x", - g.s.modTime_sec, g.s.modTime_nsec, g.s.mode, g.s.size, g.s.crc32); - if (f.modTime_sec != g.s.modTime_sec || f.modTime_nsec != g.s.modTime_nsec - || f.mode != g.s.mode || f.size != g.s.size || f.crc32 != g.s.crc32) { + LOGP("%s", q.string()); + LOGP(" old: modTime=%d,%d mode=%04o size=%-3d crc32=0x%08x", + f.modTime_sec, f.modTime_nsec, f.mode, f.size, f.crc32); + LOGP(" new: modTime=%d,%d mode=%04o size=%-3d crc32=0x%08x", + g.s.modTime_sec, g.s.modTime_nsec, g.s.mode, g.s.size, g.s.crc32); + if (f.modTime_sec != g.s.modTime_sec || f.modTime_nsec != g.s.modTime_nsec + || f.mode != g.s.mode || f.size != g.s.size || f.crc32 != g.s.crc32) { + int fd = open(g.file.string(), O_RDONLY); + if (fd < 0) { + ALOGE("Unable to read file for backup: %s", g.file.string()); + } else { write_update_file(dataStream, fd, g.s.mode, p, g.file.string()); + close(fd); } - - close(fd); } n++; m++; @@ -425,12 +428,12 @@ back_up_files(int oldSnapshotFD, BackupDataWriter* dataStream, int newSnapshotFD // these were deleted while (nWriteEntityHeader(oldSnapshot.keyAt(n), -1); + write_delete_file(dataStream, oldSnapshot.keyAt(n)); n++; } // these were added - while (m