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
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 @@
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 @@
}
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 @@
crc = crc32(crc, (Bytef*)buf, amt);
}
+ close(fd);
free(buf);
- return crc;
+
+ out->s.crc32 = crc;
+ return NO_ERROR;
}
int
@@ -353,7 +358,8 @@
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 @@
//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 @@
int n = 0;
int N = oldSnapshot.size();
int m = 0;
+ int M = newSnapshot.size();
- while (n<N && m<fileCount) {
+ while (n<N && m<M) {
const String8& p = oldSnapshot.keyAt(n);
const String8& q = newSnapshot.keyAt(m);
FileRec& g = newSnapshot.editValueAt(m);
int cmp = p.compare(q);
- if (g.deleted || cmp < 0) {
- // file removed
+ if (cmp < 0) {
+ // file present in oldSnapshot, but not present in newSnapshot
LOGP("file removed: %s", p.string());
- g.deleted = true; // They didn't mention the file, but we noticed that it's gone.
- dataStream->WriteEntityHeader(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 @@
// these were deleted
while (n<N) {
- dataStream->WriteEntityHeader(oldSnapshot.keyAt(n), -1);
+ write_delete_file(dataStream, oldSnapshot.keyAt(n));
n++;
}
// these were added
- while (m<fileCount) {
+ while (m<M) {
const String8& q = newSnapshot.keyAt(m);
FileRec& g = newSnapshot.editValueAt(m);
write_update_file(dataStream, q, g.file.string());