Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6990066:
Mac TimeMachine File Exclusions (Closed)
|
Created:
9 years, 7 months ago by mrossetti Modified:
9 years, 6 months ago CC:
chromium-reviews, pam+watch_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionMac SQLite TimeMachine File Exclusions
When an SQLite database has been excluded from Time Machine backups also exclude its -journal.
(In fts3_porter.c: had to rename the cType due to a conflict with an included Apple library.)
BUG=74053
TEST=Manually: 1) Launch browser. 2) Run the following command:
/usr/bin/xattr-2.6 ~/Library/Application\ Support/Chromium/Default/History-journal
3) Verify that the following is shown as one of the results of running the xattr-2.6 command:
com.apple.metadata:com_apple_backup_excludeItem
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87391
Patch Set 1 #
Total comments: 35
Patch Set 2 : '' #
Total comments: 20
Patch Set 3 : '' #
Total comments: 9
Patch Set 4 : '' #
Total comments: 1
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 12
Patch Set 7 : '' #
Total comments: 1
Messages
Total messages: 27 (0 generated)
Note that this patch has been tested against SQLite 3.6.18, which is the base for Chromium.
The SQLite change sort of frightens me. AFAICT, it's there because the file needs to exist before you can exclude it? Nice race condition, Apple! I believe that if you excluded the journal file in HistoryDatabase::BeginTransaction() after beginning the transaction, it would work just as well. Likewise for ThumbnailDatabase::BeginTransaction(). Unfortunately, as currently configured the journal file doesn't always exist at that point. I have to think about why. Worst case these files can have "PRAGMA journal_mode=PERSIST;", in which case the journal exists when the database exists.
I’m Not Scared. http://codereview.chromium.org/6990066/diff/1/base/mac/mac_util.mm File base/mac/mac_util.mm (right): http://codereview.chromium.org/6990066/diff/1/base/mac/mac_util.mm#newcode241 base/mac/mac_util.mm:241: bool SetFileBackupExclusion(const FilePath& file_path, bool exclude) { This entire function should be cleaned up substantially. It’s OK to do the cleanup in a future change. 1. Remove all of the temporary directory checking stuff leading up to the early return. It solves something that’s only a problem with excludeByPath and is no longer necessary with the metadata attribute-based exclude. 2. Remove the |exclude| argument and allays act as if true had been passed in. There are no callers that set it to false, except for a test. We shouldn’t need to worry about testing a behavior we never use if we can simplify things by removing support for that behavior. http://codereview.chromium.org/6990066/diff/1/base/mac/mac_util.mm#newcode279 base/mac/mac_util.mm:279: if (!success) Style nit (present in the existing code): this needs {braces} because it consumes multiple lines. http://codereview.chromium.org/6990066/diff/1/base/mac/mac_util_unittest.mm File base/mac/mac_util_unittest.mm (right): http://codereview.chromium.org/6990066/diff/1/base/mac/mac_util_unittest.mm#n... base/mac/mac_util_unittest.mm:115: EXPECT_TRUE(SetFileBackupExclusion(file_path, false)); This (and line 108) is the portion of the test that I was talking about. http://codereview.chromium.org/6990066/diff/1/chrome/browser/history/history_... File chrome/browser/history/history_database.cc (right): http://codereview.chromium.org/6990066/diff/1/chrome/browser/history/history_... chrome/browser/history/history_database.cc:100: // Exclude the history file from backups. Are we guaranteed that the -journal file does not yet exist at this point? http://codereview.chromium.org/6990066/diff/1/chrome/browser/history/thumbnai... File chrome/browser/history/thumbnail_database.cc (right): http://codereview.chromium.org/6990066/diff/1/chrome/browser/history/thumbnai... chrome/browser/history/thumbnail_database.cc:69: // Exclude the thumbnails file (and its journal) from backups. In chrome/browser/history/history_database.cc, you removed the text that you’re making parenthetical here. Which is correct? http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/pager.c File third_party/sqlite/src/src/pager.c (right): http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:127: const char kPagerJournalSuffix[] = "-journal\0"; Why have you included a \0 here? C (including derivatives like C++ and Objective-C and the bastard resulting from their union) character string literals of "double-quoted form" allays implicitly end in \0, regardless of whether or not you write it literally. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:127: const char kPagerJournalSuffix[] = "-journal\0"; However, here’s an area where C++ differs from straight C. In C++, |const| implies internal linkage, but in C, it doesn’t. Since this is a C file (and will be built as C in the amalgamation), kPagerJournalSuffix will get external linkage. We don’t want or need external linkage for this name. You need to use |static| to force this to have internal linkage, hiding it from other files. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:3999: #if defined(__APPLE__) I’m not sure what the sqlite tradition is, but it might make more sense to put this in sqliteInt.h instead of here. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4070: if( rc == SQLITE_OK && pPager->zJournal ) { sqlite style seems to not put any spaces around the == operator, and seems to nestle the opening left brace up against the closing right parenthesis, as in ){. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4073: int database_path_len = Unless there’s sqlite precedent to do otherwise… I think of “length” (and “len”) as “string length” and “size” as “buffer size.” You’re referring to the latter here, because you’re including space for the '\0' terminator, so that size = len + 1. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4077: CFURLRef database_url = sqlite seems to adhere to the rules for strict ANSI C89/ISO C90 conformance. Specifically, mixing declarations and code is forbidden. It’s OK to begin a new {scope} with declarations, and it’s OK to provide initializers for your declarations, and it’s even OK for those initializers to call other functions, but it’s not OK to do something non-declaratory and then have another declaration at the same {scope}. This line would violate that rule because you’re declaring database_url after calling strlcpy in a non-declaration. You should stick with sqlite style in here. (Personally, I think mixing declarations and non-declarations is a fantastic idea, and C99 has jumped on the bandwagon, but a quick pass through this file shows that it’s just not done in here.) http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4079: CFStringCreateWithCStringNoCopy(kCFAllocatorDefault, 1. Don’t use CFStringCreateWithCStringNoCopy with kCFStringEncodingMacRoman. This is not the right encoding to use to get a CFString referring to a filesystem path that started life as a char*. Instead, use CFStringCreateWithFileSystemRepresentation. 2. This leaks the string you create with CFStringCreateWithCStringNoCopy, or leaks the string you would create with CFStringCreateWithFileSystemRepresentation after fixing #1 above. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4081: kCFURLPOSIXPathStyle, false); Technically you should use FALSE and not false here. FALSE is the not-truthy value for a CoreFoundation capital-B Boolean, which is the type of the argument at stake. (You got it right with the TRUE and FALSE at line 4090.) http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4083: if( CSBackupIsItemExcluded(database_url, NULL) ){ I suggest checking excludeByPath here and only proceeding if it’s false. The reason is that CSBackupIsItemExcluded will return true if an ancestor of database_url is excluded by path, and I don’t think we care to exclude by metadata attribute in that case. This may happen in a temporary directory. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4086: CFStringCreateWithCStringNoCopy(kCFAllocatorDefault, The same comments I made at line 4079 above apply here, and the one from 4083 applies to 4089. You should refactor this into a common |static CFURLRef create_cfurl_from_cstring(const char*)| sort of function. Then I’d only have reason to comment once for these sorts of things. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4093: fprintf(stderr, "CSBackupSetItemExcluded(\"%s\", TRUE, FALSE): %d", It doesn’t look like sqlite actually does any sort of logging for errors of this sort. It’s probably best to just take it out since we don’t consider it critical anyway.
Drive-by with an upstreaming comment: could you send the patch upstream? Just making sure - carrying a lot of local patches to sqlite makes it harder to update.
phajdan.jr@chromium.org wrote: > Drive-by with an upstreaming comment: could you send the patch upstream? Just > making sure - carrying a lot of local patches to sqlite makes it harder to update. I suspect this wouldn’t be of interest upstream.
On 2011/05/25 13:06:32, Mark Mentovai wrote: > I suspect this wouldn’t be of interest upstream. If we don't try, we can't know. It's just more maintainable to have those changes upstream instead of porting them for each sqlite update. By the way, the changes that'll help using system sqlite for Chromium instead of the bundled version (sandboxing-related changes) are committed upstream (after some discussions, but it is possible).
http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/pager.c File third_party/sqlite/src/src/pager.c (right): http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4082: sqlite3_free(database_path); Drive-by: Use of the NoCopy functions from CF leads almost invariably to a bug. Here you use NoCopy then immediately free the backing data? :( Yes, I know that CFURLCreateWithFileSystemPath should pull the guts from the string and use it, making the deletion safe. But how do you know? For all you know, CFURL lazily computes the URL, keeping the backing string around. That's the real treachery behind NoCopy. You need to keep the backing around until the _entire_object_graph_ that uses the created object disappears. And that's a very, very long time. Mark's right: use CFStringCreateWithFileSystemRepresentation. And in the future, please never use NoCopy. It's simply not worth the risk.
Ready for the next go. http://codereview.chromium.org/6990066/diff/1/base/mac/mac_util.mm File base/mac/mac_util.mm (right): http://codereview.chromium.org/6990066/diff/1/base/mac/mac_util.mm#newcode241 base/mac/mac_util.mm:241: bool SetFileBackupExclusion(const FilePath& file_path, bool exclude) { On 2011/05/25 01:14:35, Mark Mentovai wrote: > This entire function should be cleaned up substantially. It’s OK to do the > cleanup in a future change. > > 1. Remove all of the temporary directory checking stuff leading up to the early > return. It solves something that’s only a problem with excludeByPath and is no > longer necessary with the metadata attribute-based exclude. > > 2. Remove the |exclude| argument and allays act as if true had been passed in. > There are no callers that set it to false, except for a test. We shouldn’t need > to worry about testing a behavior we never use if we can simplify things by > removing support for that behavior. Done. http://codereview.chromium.org/6990066/diff/1/base/mac/mac_util.mm#newcode279 base/mac/mac_util.mm:279: if (!success) On 2011/05/25 01:14:35, Mark Mentovai wrote: > Style nit (present in the existing code): this needs {braces} because it > consumes multiple lines. Done. http://codereview.chromium.org/6990066/diff/1/base/mac/mac_util_unittest.mm File base/mac/mac_util_unittest.mm (right): http://codereview.chromium.org/6990066/diff/1/base/mac/mac_util_unittest.mm#n... base/mac/mac_util_unittest.mm:115: EXPECT_TRUE(SetFileBackupExclusion(file_path, false)); On 2011/05/25 01:14:35, Mark Mentovai wrote: > This (and line 108) is the portion of the test that I was talking about. Done. http://codereview.chromium.org/6990066/diff/1/chrome/browser/history/history_... File chrome/browser/history/history_database.cc (right): http://codereview.chromium.org/6990066/diff/1/chrome/browser/history/history_... chrome/browser/history/history_database.cc:100: // Exclude the history file from backups. On 2011/05/25 01:14:35, Mark Mentovai wrote: > Are we guaranteed that the -journal file does not yet exist at this point? No, it might exist as a result of the app crashing. If it does exist then it will be deleted and recreated at the next transaction. Are you concerned that we might get a stale -journal that has not been excluded? http://codereview.chromium.org/6990066/diff/1/chrome/browser/history/thumbnai... File chrome/browser/history/thumbnail_database.cc (right): http://codereview.chromium.org/6990066/diff/1/chrome/browser/history/thumbnai... chrome/browser/history/thumbnail_database.cc:69: // Exclude the thumbnails file (and its journal) from backups. On 2011/05/25 01:14:35, Mark Mentovai wrote: > In chrome/browser/history/history_database.cc, you removed the text that you’re > making parenthetical here. Which is correct? I changed this comment. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/pager.c File third_party/sqlite/src/src/pager.c (right): http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:127: const char kPagerJournalSuffix[] = "-journal\0"; On 2011/05/25 01:14:35, Mark Mentovai wrote: > Why have you included a \0 here? C (including derivatives like C++ and > Objective-C and the bastard resulting from their union) character string > literals of "double-quoted form" allays implicitly end in \0, regardless of > whether or not you write it literally. Done. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:127: const char kPagerJournalSuffix[] = "-journal\0"; On 2011/05/25 01:14:35, Mark Mentovai wrote: > However, here’s an area where C++ differs from straight C. In C++, |const| > implies internal linkage, but in C, it doesn’t. Since this is a C file (and will > be built as C in the amalgamation), kPagerJournalSuffix will get external > linkage. We don’t want or need external linkage for this name. You need to use > |static| to force this to have internal linkage, hiding it from other files. Done. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:3999: #if defined(__APPLE__) On 2011/05/25 01:14:35, Mark Mentovai wrote: > I’m not sure what the sqlite tradition is, but it might make more sense to put > this in sqliteInt.h instead of here. Done. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4070: if( rc == SQLITE_OK && pPager->zJournal ) { On 2011/05/25 01:14:35, Mark Mentovai wrote: > sqlite style seems to not put any spaces around the == operator, and seems to > nestle the opening left brace up against the closing right parenthesis, as in > ){. Done. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4073: int database_path_len = On 2011/05/25 01:14:35, Mark Mentovai wrote: > Unless there’s sqlite precedent to do otherwise… > > I think of “length” (and “len”) as “string length” and “size” as “buffer size.” > You’re referring to the latter here, because you’re including space for the '\0' > terminator, so that size = len + 1. Done. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4077: CFURLRef database_url = On 2011/05/25 01:14:35, Mark Mentovai wrote: > sqlite seems to adhere to the rules for strict ANSI C89/ISO C90 conformance. > Specifically, mixing declarations and code is forbidden. It’s OK to begin a new > {scope} with declarations, and it’s OK to provide initializers for your > declarations, and it’s even OK for those initializers to call other functions, > but it’s not OK to do something non-declaratory and then have another > declaration at the same {scope}. > > This line would violate that rule because you’re declaring database_url after > calling strlcpy in a non-declaration. > > You should stick with sqlite style in here. > > (Personally, I think mixing declarations and non-declarations is a fantastic > idea, and C99 has jumped on the bandwagon, but a quick pass through this file > shows that it’s just not done in here.) Done. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4079: CFStringCreateWithCStringNoCopy(kCFAllocatorDefault, On 2011/05/25 01:14:35, Mark Mentovai wrote: > 1. Don’t use CFStringCreateWithCStringNoCopy with kCFStringEncodingMacRoman. > This is not the right encoding to use to get a CFString referring to a > filesystem path that started life as a char*. Instead, use > CFStringCreateWithFileSystemRepresentation. > > 2. This leaks the string you create with CFStringCreateWithCStringNoCopy, or > leaks the string you would create with > CFStringCreateWithFileSystemRepresentation after fixing #1 above. Done. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4081: kCFURLPOSIXPathStyle, false); On 2011/05/25 01:14:35, Mark Mentovai wrote: > Technically you should use FALSE and not false here. FALSE is the not-truthy > value for a CoreFoundation capital-B Boolean, which is the type of the argument > at stake. (You got it right with the TRUE and FALSE at line 4090.) Done. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4082: sqlite3_free(database_path); On 2011/05/25 14:22:16, Avi wrote: > Drive-by: Use of the NoCopy functions from CF leads almost invariably to a bug. > Here you use NoCopy then immediately free the backing data? :( > > Yes, I know that CFURLCreateWithFileSystemPath should pull the guts from the > string and use it, making the deletion safe. But how do you know? For all you > know, CFURL lazily computes the URL, keeping the backing string around. > > That's the real treachery behind NoCopy. You need to keep the backing around > until the _entire_object_graph_ that uses the created object disappears. And > that's a very, very long time. > > Mark's right: use CFStringCreateWithFileSystemRepresentation. And in the future, > please never use NoCopy. It's simply not worth the risk. Done. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4083: if( CSBackupIsItemExcluded(database_url, NULL) ){ On 2011/05/25 01:14:35, Mark Mentovai wrote: > I suggest checking excludeByPath here and only proceeding if it’s false. > > The reason is that CSBackupIsItemExcluded will return true if an ancestor of > database_url is excluded by path, and I don’t think we care to exclude by > metadata attribute in that case. This may happen in a temporary directory. Done. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4086: CFStringCreateWithCStringNoCopy(kCFAllocatorDefault, On 2011/05/25 01:14:35, Mark Mentovai wrote: > The same comments I made at line 4079 above apply here, and the one from 4083 > applies to 4089. > > You should refactor this into a common |static CFURLRef > create_cfurl_from_cstring(const char*)| sort of function. Then I’d only have > reason to comment once for these sorts of things. Done. http://codereview.chromium.org/6990066/diff/1/third_party/sqlite/src/src/page... third_party/sqlite/src/src/pager.c:4093: fprintf(stderr, "CSBackupSetItemExcluded(\"%s\", TRUE, FALSE): %d", On 2011/05/25 01:14:35, Mark Mentovai wrote: > It doesn’t look like sqlite actually does any sort of logging for errors of this > sort. It’s probably best to just take it out since we don’t consider it critical > anyway. Done.
Close. You’ve still got a leak. http://codereview.chromium.org/6990066/diff/1/chrome/browser/history/history_... File chrome/browser/history/history_database.cc (right): http://codereview.chromium.org/6990066/diff/1/chrome/browser/history/history_... chrome/browser/history/history_database.cc:100: // Exclude the history file from backups. mrossetti wrote: > On 2011/05/25 01:14:35, Mark Mentovai wrote: > > Are we guaranteed that the -journal file does not yet exist at this point? > > No, it might exist as a result of the app crashing. If it does exist then it > will be deleted and recreated at the next transaction. Are you concerned that we > might get a stale -journal that has not been excluded? I wasn’t worried about stale journals from previous crashed runs. I was worried that the -journal might already have been created by this point by sqlite. In that case, the sqlite code would have missed the opportunity to exclude the -journal, because the main database isn’t excluded until right here. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/README.c... File third_party/sqlite/README.chromium (right): http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/README.c... third_party/sqlite/README.chromium:21: BASE=3070603 Whether or not this is right is up to Scott. But you use $BASE and $LATEST below in the wgets, and neither http://www.sqlite.org/sqlite-3070603.tar.gz nor http://www.sqlite.org/sqlite-3070604.tar.gz seem to work for me. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/README.c... third_party/sqlite/README.chromium:183: changes were all made in pager.c. In order to eliminate a symbol conflict The change now extends to sqliteInt.h as well. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... File third_party/sqlite/src/src/pager.c (right): http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5144: CFStringCreateWithFileSystemRepresentation( You still leak the CFString created with CFStringCreateWithFileSystemRepresentation. You can CFRelease that CFString once you give it to the CFURL. If the CFURL needs to hang on to it, it’ll retain it. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5219: CFURLRef database_url = NULL; No need to initialize this to NULL or anything else. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5223: Boolean exclude_by_path; You need to declare this above, with the other declarations. For safety, since you never assign to this but hope that CSBackupIsItemExcluded sets it through the pointer, you should provide an initializer (FALSE).
I noticed problems with the Apple headers putting garbage into the global namespace in my exclusion change for safe-browsing. Maybe a better solution there would be to pull the exclusion stuff out into a file which doesn't need to have those headers in it? http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... File third_party/sqlite/src/src/pager.c (right): http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:4412: memcpy(&pPager->zJournal[nPathname], "-journal", 8); Previously you were using kPagerJournalSuffix here (I think it was here). I didn't really like it, but I could see the reason. You should either continue to use it, or add an assert() that it is the same value (and size) as the thing just appended to this buffer. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5215: if( rc == SQLITE_OK && pPager->zJournal ){ && pPager->journalMode!=PAGER_JOURNALMODE_MEMORY . Either that, or move it after the #endif a few lines up. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5222: sqlite3_free(database_path); Instead of malloc'ing up a substring, use pPager->zFilename. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5223: Boolean exclude_by_path; C code, needs to be up by database_url, though my previous suggestion may moot that. Note that the docs say "Can be NULL" for excludeByPath. Unless you pass it to the below CSBackupSetItemExcluded(), you can probably just drop it.
http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/README.c... File third_party/sqlite/README.chromium (right): http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/README.c... third_party/sqlite/README.chromium:21: BASE=3070603 On 2011/05/26 21:23:40, Mark Mentovai wrote: > Whether or not this is right is up to Scott. But you use $BASE and $LATEST below > in the wgets, and neither http://www.sqlite.org/sqlite-3070603.tar.gz nor > http://www.sqlite.org/sqlite-3070604.tar.gz seem to work for me. Needs to be .zip. And a leading 0. And the later stuff needs to be revised. And this isn't really relevant to the rest of the CL (other than Mike wanting to verify things so following these out-of-date instructions). So probably reasonable to drop this file.
http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/README.c... File third_party/sqlite/README.chromium (right): http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/README.c... third_party/sqlite/README.chromium:21: BASE=3070603 shess wrote: > On 2011/05/26 21:23:40, Mark Mentovai wrote: > > Whether or not this is right is up to Scott. But you use $BASE and $LATEST > below > > in the wgets, and neither http://www.sqlite.org/sqlite-3070603.tar.gz nor > > http://www.sqlite.org/sqlite-3070604.tar.gz seem to work for me. > > Needs to be .zip. And a leading 0. And the later stuff needs to be revised. > And this isn't really relevant to the rest of the CL (other than Mike wanting to > verify things so following these out-of-date instructions). So probably > reasonable to drop this file. Don’t drop the whole file. You can drop these changes up here, but the stuff that talks about the changes below, and that references mac_time_machine.patch, should stay.
#WINNING On Thu, May 26, 2011 at 3:14 PM, <mark@chromium.org> wrote: > > http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/README.c... > File third_party/sqlite/README.chromium (right): > > http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/README.c... > third_party/sqlite/README.chromium:21: BASE=3070603 > shess wrote: >> >> On 2011/05/26 21:23:40, Mark Mentovai wrote: >> > Whether or not this is right is up to Scott. But you use $BASE and > > $LATEST >> >> below >> > in the wgets, and neither > > http://www.sqlite.org/sqlite-3070603.tar.gz nor >> >> > http://www.sqlite.org/sqlite-3070604.tar.gz seem to work for me. > >> Needs to be .zip. And a leading 0. And the later stuff needs to be > > revised. >> >> And this isn't really relevant to the rest of the CL (other than Mike > > wanting to >> >> verify things so following these out-of-date instructions). So > > probably >> >> reasonable to drop this file. > > Don’t drop the whole file. You can drop these changes up here, but the > stuff that talks about the changes below, and that references > mac_time_machine.patch, should stay. > > http://codereview.chromium.org/6990066/ >
Mark, do you have a different opinion about the exclude-by-path comment I made in pager.c? http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/README.c... File third_party/sqlite/README.chromium (right): http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/README.c... third_party/sqlite/README.chromium:21: BASE=3070603 On 2011/05/26 22:14:22, Mark Mentovai wrote: > shess wrote: > > On 2011/05/26 21:23:40, Mark Mentovai wrote: > > > Whether or not this is right is up to Scott. But you use $BASE and $LATEST > > below > > > in the wgets, and neither http://www.sqlite.org/sqlite-3070603.tar.gz nor > > > http://www.sqlite.org/sqlite-3070604.tar.gz seem to work for me. > > > > Needs to be .zip. And a leading 0. And the later stuff needs to be revised. > > And this isn't really relevant to the rest of the CL (other than Mike wanting > to > > verify things so following these out-of-date instructions). So probably > > reasonable to drop this file. > > Don’t drop the whole file. You can drop these changes up here, but the stuff > that talks about the changes below, and that references mac_time_machine.patch, > should stay. I removed the version stuff for $BASE and $LATEST because while the $BASE was correct (with the .zip change) the $LATEST couldn't be found. Best left to shess. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/README.c... third_party/sqlite/README.chromium:183: changes were all made in pager.c. In order to eliminate a symbol conflict On 2011/05/26 21:23:40, Mark Mentovai wrote: > The change now extends to sqliteInt.h as well. Done. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... File third_party/sqlite/src/src/pager.c (right): http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:4412: memcpy(&pPager->zJournal[nPathname], "-journal", 8); On 2011/05/26 21:30:54, shess wrote: > Previously you were using kPagerJournalSuffix here (I think it was here). I > didn't really like it, but I could see the reason. You should either continue > to use it, or add an assert() that it is the same value (and size) as the thing > just appended to this buffer. Gah! Merge problem. Fixed. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5144: CFStringCreateWithFileSystemRepresentation( On 2011/05/26 21:23:40, Mark Mentovai wrote: > You still leak the CFString created with > CFStringCreateWithFileSystemRepresentation. You can CFRelease that CFString once > you give it to the CFURL. If the CFURL needs to hang on to it, it’ll retain it. Done. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5215: if( rc == SQLITE_OK && pPager->zJournal ){ On 2011/05/26 21:30:54, shess wrote: > && pPager->journalMode!=PAGER_JOURNALMODE_MEMORY . Either that, or move it > after the #endif a few lines up. Done. Moved up since there is no need to mark it excluded when in memory. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5219: CFURLRef database_url = NULL; On 2011/05/26 21:23:40, Mark Mentovai wrote: > No need to initialize this to NULL or anything else. Done. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5222: sqlite3_free(database_path); On 2011/05/26 21:30:54, shess wrote: > Instead of malloc'ing up a substring, use pPager->zFilename. Done. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5223: Boolean exclude_by_path; On 2011/05/26 21:23:40, Mark Mentovai wrote: > You need to declare this above, with the other declarations. For safety, since > you never assign to this but hope that CSBackupIsItemExcluded sets it through > the pointer, you should provide an initializer (FALSE). Done. http://codereview.chromium.org/6990066/diff/10001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5223: Boolean exclude_by_path; On 2011/05/26 21:30:54, shess wrote: > C code, needs to be up by database_url, though my previous suggestion may moot > that. Note that the docs say "Can be NULL" for excludeByPath. Unless you pass > it to the below CSBackupSetItemExcluded(), you can probably just drop it. I'm removing it. Previously we discussed not excluding the journal if the database was excluded by-path but that doesn't make sense to me anymore. If the database is excluded by path it's either because a) the full path for the database has been excluded or b) a partial path referring to a containing directory has been excluded. In case 'a' we still want to force the journal to be excluded and in case 'b' the exclusion is potentially unnecessary (if the exclusion applies to a containing directory) but harmless.
mrossetti@chromium.org wrote: > Mark, do you have a different opinion about the exclude-by-path comment I > made in pager.c? [...] > I'm removing it. Previously we discussed not excluding the journal if > the database was excluded by-path but that doesn't make sense to me > anymore. If the database is excluded by path it's either because a) the > full path for the database has been excluded or b) a partial path > referring to a containing directory has been excluded. In case 'a' we > still want to force the journal to be excluded and in case 'b' the > exclusion is potentially unnecessary (if the exclusion applies to a > containing directory) but harmless. I guess that’s fine if you’re not doing the unexclude stuff for the -journal, but remember that I felt pretty strongly that we should clean up our mess. That should address case “a.” I’d prefer leaving the check in place. It doesn’t hurt and it’s “righter” in case “b.”
LG otherwise. http://codereview.chromium.org/6990066/diff/17001/third_party/sqlite/README.c... File third_party/sqlite/README.chromium (right): http://codereview.chromium.org/6990066/diff/17001/third_party/sqlite/README.c... third_party/sqlite/README.chromium:184: sqliteInt.h. In order to eliminate a symbol conflict with an Apple library Extra space between “conflict” and “with.” http://codereview.chromium.org/6990066/diff/17001/third_party/sqlite/src/src/... File third_party/sqlite/src/src/pager.c (right): http://codereview.chromium.org/6990066/diff/17001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5144: CFString url_string = CFStringCreateWithFileSystemRepresentation( CFStringRef, not CFString. This wouldn’t even have compiled successfully. http://codereview.chromium.org/6990066/diff/17001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5219: if( CSBackupIsItemExcluded(database_url, NULL) ){ As discussed.
On 2011/05/27 17:17:14, Mark Mentovai wrote: > mailto:mrossetti@chromium.org wrote: > > Mark, do you have a different opinion about the exclude-by-path comment I > > made in pager.c? > [...] > > I'm removing it. Previously we discussed not excluding the journal if > > the database was excluded by-path but that doesn't make sense to me > > anymore. If the database is excluded by path it's either because a) the > > full path for the database has been excluded or b) a partial path > > referring to a containing directory has been excluded. In case 'a' we > > still want to force the journal to be excluded and in case 'b' the > > exclusion is potentially unnecessary (if the exclusion applies to a > > containing directory) but harmless. > > I guess that’s fine if you’re not doing the unexclude stuff for the > -journal, but remember that I felt pretty strongly that we should > clean up our mess. That should address case “a.” > > I’d prefer leaving the check in place. It doesn’t hurt and it’s > “righter” in case “b.” I do not fully understand what's going on here, but as far as I do understand it, if the main database file has been excluded by path, then Chrome did not do it. If that's a correct understanding, then I'd be perfectly happen letting whoever made that setting worry about the journal file rather than adding our own "intelligence" to the issue. http://codereview.chromium.org/6990066/diff/17001/third_party/sqlite/src/src/... File third_party/sqlite/src/src/pager.c (right): http://codereview.chromium.org/6990066/diff/17001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5215: if( rc == SQLITE_OK && pPager->zJournal ){ In this location, I don't know what it would mean for zJournal to be NULL. If it can be NULL here, and it was successfully opened ... can zFilename also be NULL? Maybe zJournal is only NULL for a memory journal? http://codereview.chromium.org/6990066/diff/17001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5217: - strlen(kPagerJournalSuffix) + 1; database_path_size now unused.
shess@chromium.org wrote: > I do not fully understand what's going on here, but as far as I do understand > it, if the main database file has been excluded by path, then Chrome did not do > it. If that's a correct understanding, then I'd be perfectly happen letting > whoever made that setting worry about the journal file rather than adding > our own "intelligence" to the issue. At this point, I think either is fine. I’ve indicated my preference based on the idea that Chrome shouldn’t exclude the -journal if it didn’t exclude the main database, but it’s just a preference and if Mike would prefer to exclude the -journal if the main database is excluded by path or by metadata attribute, that’s OK too.
I believe Mark and I are in agreement with excluding the -journal !by-path if the database is excluded regardless of how the database is excluded. I will be submitting a separate patch to perform the clean-up of the potential exclude-by-path of the -journal caused by last year's misdirected exclusion efforts. I have applied all of the Chrome SQLite patches, including the new mac_time_machine.patch, against a virgin 3.7.6.3 SQLite and verified that the results are as expected. shess, if all looks good to you then I'll get this committed. Future upstreaming should be considered once this has had an opportunity to 'bake' for a while. http://codereview.chromium.org/6990066/diff/17001/third_party/sqlite/README.c... File third_party/sqlite/README.chromium (right): http://codereview.chromium.org/6990066/diff/17001/third_party/sqlite/README.c... third_party/sqlite/README.chromium:184: sqliteInt.h. In order to eliminate a symbol conflict with an Apple library On 2011/05/27 17:20:37, Mark Mentovai wrote: > Extra space between “conflict” and “with.” Done. http://codereview.chromium.org/6990066/diff/17001/third_party/sqlite/src/src/... File third_party/sqlite/src/src/pager.c (right): http://codereview.chromium.org/6990066/diff/17001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5144: CFString url_string = CFStringCreateWithFileSystemRepresentation( On 2011/05/27 17:20:37, Mark Mentovai wrote: > CFStringRef, not CFString. > > This wouldn’t even have compiled successfully. Yeah, my local build bashed me over the head with this about 12 seconds after I pressed "Send Comments Prematurely". http://codereview.chromium.org/6990066/diff/17001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5215: if( rc == SQLITE_OK && pPager->zJournal ){ On 2011/05/27 17:23:34, shess wrote: > In this location, I don't know what it would mean for zJournal to be NULL. If > it can be NULL here, and it was successfully opened ... can zFilename also be > NULL? Maybe zJournal is only NULL for a memory journal? Done. http://codereview.chromium.org/6990066/diff/17001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5217: - strlen(kPagerJournalSuffix) + 1; On 2011/05/27 17:23:34, shess wrote: > database_path_size now unused. Done.
LGTM. BTW, one thing I am considering in rewriting the README.Chromium is whether I shouldn't just checkin the .zip file downloaded from SQLite and rephrase things as a script which unpacks things and applies the patches. Clearly that would make your current case easier. On the flip side, if they change their packaging again, then to import you have to fix the script which doesn't work before you can move forward (the current import is phrased as a README specifically to leverage having an intelligent human in the loop). Since you've now essentially gone through doing an import, WDYT about these tradeoffs? http://codereview.chromium.org/6990066/diff/23001/third_party/sqlite/src/src/... File third_party/sqlite/src/src/pager.c (right): http://codereview.chromium.org/6990066/diff/23001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:434: static const char kPagerJournalSuffix[] = "-journal"; Guess what? Now that you aren't deconstructing the filename, this variable is only used in the other place where you changed SQLite! Your option as to whether to manage it as a clean-up CL.
LGTM (from a 3-inch screen, but I think I was able to read everything.) I agree that having "-journal" factored out into its own constant is now out of scope for this change. I also agree that you can check this in as-is and fix that in a follow-up if you prefer. Thanks for taking care of this! On May 27, 2011 3:32 PM, <shess@chromium.org> wrote: > LGTM. > > BTW, one thing I am considering in rewriting the README.Chromium is whether > I > shouldn't just checkin the .zip file downloaded from SQLite and rephrase > things > as a script which unpacks things and applies the patches. Clearly that > would > make your current case easier. On the flip side, if they change their > packaging > again, then to import you have to fix the script which doesn't work before > you > can move forward (the current import is phrased as a README specifically to > leverage having an intelligent human in the loop). > > Since you've now essentially gone through doing an import, WDYT about these > tradeoffs? > > > http://codereview.chromium.org/6990066/diff/23001/third_party/sqlite/src/src/... > File third_party/sqlite/src/src/pager.c (right): > > http://codereview.chromium.org/6990066/diff/23001/third_party/sqlite/src/src/... > third_party/sqlite/src/src/pager.c:434: static const char > kPagerJournalSuffix[] = "-journal"; > Guess what? Now that you aren't deconstructing the filename, this > variable is only used in the other place where you changed SQLite! Your > option as to whether to manage it as a clean-up CL. > > http://codereview.chromium.org/6990066/
On 2011/05/27 19:32:43, shess wrote: > LGTM. > > BTW, one thing I am considering in rewriting the README.Chromium is whether I > shouldn't just checkin the .zip file downloaded from SQLite and rephrase things > as a script which unpacks things and applies the patches. Clearly that would > make your current case easier. On the flip side, if they change their packaging > again, then to import you have to fix the script which doesn't work before you > can move forward (the current import is phrased as a README specifically to > leverage having an intelligent human in the loop). > > Since you've now essentially gone through doing an import, WDYT about these > tradeoffs? Yes, I'd prefer having the .zip checked in and believe it would reduce the possibility of syncing our patches up against the wrong version of SQLite. Of course, it's quickly obvious that a version mismatch has occurred (as happened with me the first time through) and it's no tough cookie to fix. OTOH, I'd consider having a separate section in the README which describes the procedure suggested for updating the SQLite version upon which we depend. > http://codereview.chromium.org/6990066/diff/23001/third_party/sqlite/src/src/... > File third_party/sqlite/src/src/pager.c (right): > > http://codereview.chromium.org/6990066/diff/23001/third_party/sqlite/src/src/... > third_party/sqlite/src/src/pager.c:434: static const char kPagerJournalSuffix[] > = "-journal"; > Guess what? Now that you aren't deconstructing the filename, this variable is > only used in the other place where you changed SQLite! Your option as to > whether to manage it as a clean-up CL. I'd prefer to not wait to clean this up and so have made that change. Simple enough now that I have been through the process a few times.
Still LGTM! I promise not to intentionally nominate you for the next SQLite import :-).
Sticky LGTM
I was letting the style go, because I'm honestly not entirely sure I can mentally host it any more, but here goes ... http://codereview.chromium.org/6990066/diff/25001/third_party/sqlite/src/src/... File third_party/sqlite/src/src/pager.c (right): http://codereview.chromium.org/6990066/diff/25001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5141: url_string, kCFURLPOSIXPathStyle, FALSE); I am not certain how to translate this kind of thing into the SQLite variable-naming style, which is kind of Hungarian, and definitely not under_bars. file_path -> zPathname or zFilename is certainly good. Maybe url_string -> stringRef and url_ref -> urlRef would be in the right spirit. Function name is probably alright. http://codereview.chromium.org/6990066/diff/25001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5208: // been set for the database. /* */ comments. http://codereview.chromium.org/6990066/diff/25001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5210: && strlen(pPager->zFilename) > 0){ rc==SQLITE_OK (no spaces), spaces inside the last paren. SQLite's style is more informal than ours, and it's basically opposite WRT spaces in some places :-). http://codereview.chromium.org/6990066/diff/25001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5211: CFURLRef database_url = create_cfurl_from_cstring(pPager->zFilename); Maybe just |database|. http://codereview.chromium.org/6990066/diff/25001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5213: CFURLRef journal_url = create_cfurl_from_cstring(pPager->zJournal); And |journal|. http://codereview.chromium.org/6990066/diff/25001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5214: // Ignore errors from the following exclusion call. /* comment */
Just to let you know that I cleaned up the style issues. http://codereview.chromium.org/6990066/diff/25001/third_party/sqlite/src/src/... File third_party/sqlite/src/src/pager.c (right): http://codereview.chromium.org/6990066/diff/25001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5141: url_string, kCFURLPOSIXPathStyle, FALSE); On 2011/05/31 19:52:25, shess wrote: > I am not certain how to translate this kind of thing into the SQLite > variable-naming style, which is kind of Hungarian, and definitely not > under_bars. file_path -> zPathname or zFilename is certainly good. Maybe > url_string -> stringRef and url_ref -> urlRef would be in the right spirit. > Function name is probably alright. Done. http://codereview.chromium.org/6990066/diff/25001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5208: // been set for the database. On 2011/05/31 19:52:25, shess wrote: > /* */ comments. Done. http://codereview.chromium.org/6990066/diff/25001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5210: && strlen(pPager->zFilename) > 0){ On 2011/05/31 19:52:25, shess wrote: > rc==SQLITE_OK (no spaces), spaces inside the last paren. SQLite's style is more > informal than ours, and it's basically opposite WRT spaces in some places :-). Done. http://codereview.chromium.org/6990066/diff/25001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5211: CFURLRef database_url = create_cfurl_from_cstring(pPager->zFilename); On 2011/05/31 19:52:25, shess wrote: > Maybe just |database|. Done. http://codereview.chromium.org/6990066/diff/25001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5213: CFURLRef journal_url = create_cfurl_from_cstring(pPager->zJournal); On 2011/05/31 19:52:25, shess wrote: > And |journal|. Done. http://codereview.chromium.org/6990066/diff/25001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5214: // Ignore errors from the following exclusion call. On 2011/05/31 19:52:25, shess wrote: > /* comment */ Done.
LGTM. BTW, if you're interested in style for purposes of submitting the patch upstream, note that often such patches get rewritten/refactored by the SQLite team anyhow. So I wouldn't go too far in getting it perfect. http://codereview.chromium.org/6990066/diff/30001/third_party/sqlite/src/src/... File third_party/sqlite/src/src/pager.c (right): http://codereview.chromium.org/6990066/diff/30001/third_party/sqlite/src/src/... third_party/sqlite/src/src/pager.c:5137: static CFURLRef create_cfurl_from_cstring(const char* filePath){ I'm fine with this, but pedantic would be zFilename or zPathname. z == nul-terminated char*. Also, looking around, looks like SQLite has the space on the left of the * ("const char *zFilename" rather than "const char* zFilename"). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
