Mac TimeMachine File Exclusions

third_party/sqlite/src/src/pager.c

Index: third_party/sqlite/src/src/pager.c
===================================================================
--- third_party/sqlite/src/src/pager.c	(revision 86429)
+++ third_party/sqlite/src/src/pager.c	(working copy)
@@ -122,6 +122,11 @@
 #define MAX_SECTOR_SIZE 0x10000
 
 /*
+** The string appended to the database file name giving the journal file name.
+*/
+const char kPagerJournalSuffix[] = "-journal\0";

[Mark Mentovai, 2011/05/25 01:14:35]

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.

[Mark Mentovai, 2011/05/25 01:14:35]

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.

[mrossetti, 2011/05/26 21:02:40]

Done.

[mrossetti, 2011/05/26 21:02:40]

Done.

+
+/*
 ** An instance of the following structure is allocated for each active
 ** savepoint and statement transaction in the system. All such structures
 ** are stored in the Pager.aSavepoint[] array, which is allocated and
@@ -3222,7 +3227,8 @@
     pPager->zJournal =   (char*)(pPtr += nPathname + 1);
     memcpy(pPager->zFilename, zPathname, nPathname);
     memcpy(pPager->zJournal, zPathname, nPathname);
-    memcpy(&pPager->zJournal[nPathname], "-journal", 8);
+    memcpy(&pPager->zJournal[nPathname], kPagerJournalSuffix,
+           strlen(kPagerJournalSuffix));
     if( pPager->zFilename[0]==0 ) pPager->zJournal[0] = 0;
     sqlite3_free(zPathname);
   }
@@ -3990,6 +3996,11 @@
   return rc;
 }
 
+#if defined(__APPLE__)

[Mark Mentovai, 2011/05/25 01:14:35]

I’m not sure what the sqlite tradition is, but it might make more sense to put
this in sqliteInt.h instead of here.

[mrossetti, 2011/05/26 21:02:40]

Done.

+#include <CoreServices/CoreServices.h>
+#include <CoreFoundation/CoreFoundation.h>
+#endif
+
 /*
 ** This function is called at the start of every write transaction.
 ** There must already be a RESERVED or EXCLUSIVE lock on the database 
@@ -4055,6 +4066,38 @@
 #else
       rc = sqlite3OsOpen(pVfs, pPager->zJournal, pPager->jfd, flags, 0);
 #endif
+#if defined(__APPLE__)
+      if( rc == SQLITE_OK && pPager->zJournal ) {

[Mark Mentovai, 2011/05/25 01:14:35]

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
){.

[mrossetti, 2011/05/26 21:02:40]

Done.

+        // Set the TimeMachine exclusion metadata for the journal if it has
+        // been set for the database.
+        int database_path_len =

[Mark Mentovai, 2011/05/25 01:14:35]

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.

[mrossetti, 2011/05/26 21:02:40]

Done.

+            strlen(pPager->zJournal) - strlen(kPagerJournalSuffix) + 1;
+        char *database_path = sqlite3Malloc(database_path_len);
+        strlcpy(database_path, pPager->zJournal, database_path_len);
+        CFURLRef database_url =

[Mark Mentovai, 2011/05/25 01:14:35]

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.)

[mrossetti, 2011/05/26 21:02:40]

Done.

+            CFURLCreateWithFileSystemPath(kCFAllocatorDefault,
+                CFStringCreateWithCStringNoCopy(kCFAllocatorDefault,

[Mark Mentovai, 2011/05/25 01:14:35]

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.

[mrossetti, 2011/05/26 21:02:40]

Done.

+                    database_path, kCFStringEncodingMacRoman, kCFAllocatorNull),
+                kCFURLPOSIXPathStyle, false);

[Mark Mentovai, 2011/05/25 01:14:35]

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.)

[mrossetti, 2011/05/26 21:02:40]

Done.

+        sqlite3_free(database_path);

[Avi (use Gerrit), 2011/05/25 14:22:16]

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.

[mrossetti, 2011/05/26 21:02:40]

Done.

+        if( CSBackupIsItemExcluded(database_url, NULL) ){

[Mark Mentovai, 2011/05/25 01:14:35]

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.

[mrossetti, 2011/05/26 21:02:40]

Done.

+          CFURLRef journal_url =
+              CFURLCreateWithFileSystemPath(kCFAllocatorDefault,
+                  CFStringCreateWithCStringNoCopy(kCFAllocatorDefault,

[Mark Mentovai, 2011/05/25 01:14:35]

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.

[mrossetti, 2011/05/26 21:02:40]

Done.

+                      pPager->zJournal, kCFStringEncodingMacRoman,
+                      kCFAllocatorNull),
+                  kCFURLPOSIXPathStyle, false);
+          OSStatus err = CSBackupSetItemExcluded(journal_url, TRUE, FALSE);
+          CFRelease(journal_url);
+          if( err != noErr ){
+            fprintf(stderr, "CSBackupSetItemExcluded(\"%s\", TRUE, FALSE): %d",

[Mark Mentovai, 2011/05/25 01:14:35]

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.

[mrossetti, 2011/05/26 21:02:40]

Done.

+                pPager->zJournal, (int)err);
+            // DO NOT set rc to an error code -- ignore exclusion errors.
+          }
+        }
+        CFRelease(database_url);
+      }
+#endif
     }
     assert( rc!=SQLITE_OK || isOpen(pPager->jfd) );
   }