changeset 400:8c7453fb5bd9 api-inversion

Invert audioDB::power_flag / audiodb_power()

Here the exciting discovery is that the mmap(), memcpy(), munmap()
sequence is in fact not safe.  In principle an msync() call should be
inserted before unmapping for in-core changes to mmap()ed files to be
flushed to disk.

In this case we work around the problem entirely, by not mmap()ing
anything and doing everything with file descriptors.  Amusingly, that's
probably not desperately safe either, this time because we have to move
the file descriptor position (which is also a shared resource).  dup()
doesn't save us, as the duplicate file descriptor shares a file
position.

This applies also to the filling of data_buffer in the query loop, and
in fact basically any call to lseek(), which is why I'm not fixing it
now.  Solution: if you have multiple threads all acting at once on a
single database, do one audiodb_open() per thread, for now at least.

--- a/Makefile	Thu Nov 27 15:19:49 2008 +0000
+++ b/Makefile	Thu Nov 27 16:22:52 2008 +0000
@@ -8,7 +8,7 @@
 
 SHARED_LIB_FLAGS=-shared -Wl,-soname,
 
-LIBOBJS=insert.o create.o common.o open.o close.o status.o dump.o liszt.o query.o sample.o index.o lshlib.o cmdline.o
+LIBOBJS=insert.o create.o common.o open.o close.o status.o dump.o power.o liszt.o query.o sample.o index.o lshlib.o cmdline.o
 OBJS=$(LIBOBJS) soap.o audioDB.o
 
 

--- a/audioDB.cpp	Thu Nov 27 15:19:49 2008 +0000
+++ b/audioDB.cpp	Thu Nov 27 16:22:52 2008 +0000
@@ -771,13 +771,14 @@
 }
 
 void audioDB::power_flag(const char *dbName) {
-  forWrite = true;
-  initTables(dbName, 0);  
-  if( !(dbH->flags & O2_FLAG_LARGE_ADB ) && (dbH->length>0) ){
-    error("cannot turn on power storage for non-empty database", dbName);
+  if(!adb) {
+    if(!(adb = audiodb_open(dbName, O_RDWR))) {
+      error("Failed to open database file", dbName);
+    }
   }
-  dbH->flags |= O2_FLAG_POWER;
-  memcpy(db, dbH, O2_HEADERSIZE);
+  if(audiodb_power(adb)) {
+    error("can't turn on power flag for database", dbName);
+  }
 }
 
 void audioDB::create(const char *dbName) {
@@ -1154,20 +1155,5 @@
       audioDB::audioDB(4,argv,&apierror,mydb);
       return apierror;
   }
-
-  int audiodb_power(adb_ptr mydb){
-
-      const char *argv[5];
-      int apierror=0;
-
-      argv[0]="audioDB";
-      argv[1]="--POWER";
-      argv[2]="-d";
-      argv[3]=mydb->path;
-      argv[4]='\0';
-
-      audioDB::audioDB(4,argv,&apierror,mydb);
-      return apierror;
-  }
 }
 

--- a/audioDB_API.h	Thu Nov 27 15:19:49 2008 +0000
+++ b/audioDB_API.h	Thu Nov 27 16:22:52 2008 +0000
@@ -13,6 +13,26 @@
 typedef struct dbTableHeader adb_header_t;
 int acquire_lock(int, bool);
 
+/* Internal Macros: make sure they end up in the private version of
+   this header. */
+
+/* We could go gcc-specific here and use typeof() instead of passing
+ * in an explicit type.  Answers on a postcard as to whether that's a
+ * good plan or not. */
+#define mmap_or_goto_error(type, var, start, length) \
+  { void *tmp = mmap(0, length, PROT_READ, MAP_SHARED, adb->fd, (start)); \
+    if(tmp == (void *) -1) { \
+      goto error; \
+    } \
+    var = (type) tmp; \
+  }
+
+#define maybe_munmap(table, length) \
+  { if(table) { \
+      munmap(table, length); \
+    } \
+  }
+
 /* The main struct that stores the name of the database, and in future will hold all
  * kinds of other interesting information */
 /* This basically gets passed around to all of the other functions */

--- a/dump.cpp	Thu Nov 27 15:19:49 2008 +0000
+++ b/dump.cpp	Thu Nov 27 16:22:52 2008 +0000
@@ -3,23 +3,6 @@
 #include "audioDB_API.h"
 }
 
-/* We could go gcc-specific here and use typeof() instead of passing
- * in an explicit type.  Answers on a postcard as to whether that's a
- * good plan or not. */
-#define mmap_or_goto_error(type, var, start, length) \
-  { void *tmp = mmap(0, length, PROT_READ, MAP_SHARED, adb->fd, (start)); \
-    if(tmp == (void *) -1) { \
-      goto error; \
-    } \
-    var = (type) tmp; \
-  }
-
-#define maybe_munmap(table, length) \
-  { if(table) { \
-      munmap(table, length); \
-    } \
-  }
-
 int audiodb_dump(adb_t *adb, const char *output) {
   char *fileTable = 0; /* key_table */
   unsigned *trackTable = 0; /* track_size_table */

--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/power.cpp	Thu Nov 27 16:22:52 2008 +0000
@@ -0,0 +1,41 @@
+#include "audioDB.h"
+extern "C" {
+#include "audioDB_API.h"
+}
+
+/* FIXME: we should not export this symbol to users of the library. */
+int audiodb_sync_header(adb_t *adb) {
+  off_t pos;
+  pos = lseek(adb->fd, (off_t) 0, SEEK_CUR);
+  if(pos == (off_t) -1) {
+    goto error;
+  }
+  if(lseek(adb->fd, (off_t) 0, SEEK_SET) == (off_t) -1) {
+    goto error;
+  }
+  if(write(adb->fd, adb->header, O2_HEADERSIZE) != O2_HEADERSIZE) {
+    goto error;
+  }
+
+  /* can be fsync() if fdatasync() is racily exciting and new */
+  fdatasync(adb->fd);
+  if(lseek(adb->fd, pos, SEEK_SET) == (off_t) -1) {
+    goto error;
+  }
+  return 0;
+
+ error:
+  return 1;
+}
+
+int audiodb_power(adb_t *adb) {
+  /* FIXME: we should probably include in adb_t information about
+   * which mode (O_RDONLY|O_RDWR) the database was opened, so that we
+   * can check that it's writeable. */
+  if(adb->header->length > 0) {
+    return 1;
+  }
+
+  adb->header->flags |= O2_FLAG_POWER;
+  return audiodb_sync_header(adb);
+}