changeset 392:78fed0d4c108 api-inversion

Include some necessary information in struct adb.

Now the struct adb contains a database fd, the flags used to open that
fd (so that we can later tell if it was for write or not) and a database
header pointer.

audiodb_open() is now responsible for filling in all of that
information.  To do that, it needs to take an open(2) flag; that's good,
because it means that the call to open(2) is no longer invoking
undefined behaviour.  (Also, the previous version of audiodb_open()
leaked an fd).  Unfortunately, that means we have broken ABI and API
compatibility.  (Fortunately, we have fewer than 12 users).

Use audiodb_open() in audioDB::initDBHeader().

We've temporarily(?) put acquire_lock(int, bool) in the API header; that
means we need to include <stdbool.h> and compile C files with -std=c99.
Do so.

Make audiodb_close() free resources allocated by audiodb_open().

Include a struct adb * field in the audioDB C++ object...
... which lets us actually implement memory-correctness, by
audiodb_close()ing the database in audioDB::cleanup().

[ The lock is, I think, correctly disposed of; man fcntl(2) on Linux
  says that the locks are released once any file descriptor relating to
  the file is closed, and we close the fd in audiodb_close(). ]

--- a/Makefile	Mon Nov 24 12:50:38 2008 +0000
+++ b/Makefile	Mon Nov 24 15:42:15 2008 +0000
@@ -8,9 +8,7 @@
 
 SHARED_LIB_FLAGS=-shared -Wl,-soname,
 
-
-
-LIBOBJS=insert.o create.o common.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 dump.o liszt.o query.o sample.o index.o lshlib.o cmdline.o
 OBJS=$(LIBOBJS) soap.o audioDB.o
 
 

--- a/audioDB.cpp	Mon Nov 24 12:50:38 2008 +0000
+++ b/audioDB.cpp	Mon Nov 24 15:42:15 2008 +0000
@@ -333,12 +333,12 @@
     gsl_rng_free(rng);
   if(vv)
     delete vv;
-  if(dbfid>0)
-    close(dbfid);
   if(infid>0)
     close(infid);
-  if(dbH)
-    delete dbH;
+  if(adb) {
+    audiodb_close(adb);
+    adb = NULL;
+  }
   if(lsh!=SERVER_LSH_INDEX_SINGLETON)
     delete lsh;
 }
@@ -846,7 +846,7 @@
 void audioDB::create(const char *dbName) {
   adb_t *adb;
   if(!(adb = audiodb_create(dbName, datasize, ntracks, datadim))) {
-    error("Failed to create database file", dbName, "");
+    error("Failed to create database file", dbName);
   }
   audiodb_close(adb);
 }
@@ -1270,37 +1270,5 @@
       audioDB::audioDB(4,argv,&apierror);
       return apierror;
   }
-
-  adb_ptr audiodb_open(const char *path){
-
-        adb_ptr mydbp;
-        adbstatus mystatus;
-
-        /* if db exists */
-
-        if (open(path, O_EXCL) != -1){
-
-            mydbp=(adb_ptr)malloc(sizeof(adb));
-            mydbp->path=(char *)malloc(1+strlen(path));
-
-            strcpy(mydbp->path,path); 
-
-            audiodb_status(mydbp, &mystatus);
-            return mydbp;
-        }
-
-        return NULL;
-  };
-
-
-
-  void audiodb_close(adb_ptr db){
-
-      free(db->path);
-      free(db);
-
-  }
-
-
 }
 

--- a/audioDB.h	Mon Nov 24 12:50:38 2008 +0000
+++ b/audioDB.h	Mon Nov 24 15:42:15 2008 +0000
@@ -254,6 +254,7 @@
   char* indata;
   struct stat statbuf;  
   dbTableHeaderPtr dbH;
+  struct adb *adb;
 
   gsl_rng *rng;
   
@@ -462,6 +463,7 @@
     db(0),					\
     indata(0),					\
     dbH(0),					\
+    adb(0),                                     \
     rng(0),                                     \
     fileTable(0),				\
     trackTable(0),				\

--- a/audioDB_API.h	Mon Nov 24 12:50:38 2008 +0000
+++ b/audioDB_API.h	Mon Nov 24 15:42:15 2008 +0000
@@ -1,3 +1,4 @@
+#include <stdbool.h>
 /* for API questions contact 
  * Christophe Rhodes [e-mail]
  * Ian Knopke [e-mail], [e-mail] */
@@ -6,7 +7,9 @@
 /*******************************************************************/
 /* Data types for API */
 
+/* Temporary workarounds */
 typedef struct dbTableHeader adb_header_t;
+int acquire_lock(int, bool);
 
 /* The main struct that stores the name of the database, and in future will hold all
  * kinds of other interesting information */
@@ -15,6 +18,7 @@
 struct adb {
   char *path;
   int fd;
+  int flags;
   adb_header_t *header;
 };
 /* FIXME: it might be that "adb_" isn't such a good prefix to use, and
@@ -89,7 +93,7 @@
 
 /* open an existing database */
 /* returns a struct or NULL on failure */
-adb_ptr audiodb_open(const char *path);
+adb_ptr audiodb_open(const char *path, int flags);
 
 /* create a new database */
 /* returns a struct or NULL on failure */

--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/close.cpp	Mon Nov 24 15:42:15 2008 +0000
@@ -0,0 +1,11 @@
+#include "audioDB.h"
+extern "C" {
+#include "audioDB_API.h"
+}
+
+void audiodb_close(adb_t *adb) {
+  free(adb->path);
+  free(adb->header);
+  close(adb->fd);
+  free(adb);
+}

--- a/common.cpp	Mon Nov 24 12:50:38 2008 +0000
+++ b/common.cpp	Mon Nov 24 15:42:15 2008 +0000
@@ -1,4 +1,7 @@
 #include "audioDB.h"
+extern "C" {
+#include "audioDB_API.h"
+}
 
 #if defined(O2_DEBUG)
 void sigterm_action(int signal, siginfo_t *info, void *context) {
@@ -95,37 +98,12 @@
 }
 
 void audioDB::initDBHeader(const char* dbName) {
-  if ((dbfid = open(dbName, forWrite ? O_RDWR : O_RDONLY)) < 0) {
-    error("Can't open database file", dbName, "open");
+  adb = audiodb_open(dbName, forWrite ? O_RDWR : O_RDONLY);
+  if(!adb) {
+    error("Failed to open database", dbName);
   }
-
-  get_lock(dbfid, forWrite);
-  // Get the database header info
-  dbH = new dbTableHeaderT();
-  assert(dbH);
-  
-  if(read(dbfid, (char *) dbH, O2_HEADERSIZE) != O2_HEADERSIZE) {
-    error("error reading db header", dbName, "read");
-  }
-
-  if(dbH->magic == O2_OLD_MAGIC) {
-    // FIXME: if anyone ever complains, write the program to convert
-    // from the old audioDB format to the new...
-    error("database file has old O2 header", dbName);
-  }
-
-  if(dbH->magic != O2_MAGIC) {
-    std::cerr << "expected: " << O2_MAGIC << ", got: " << dbH->magic << std::endl;
-    error("database file has incorrect header", dbName);
-  }
-
-  if(dbH->version != O2_FORMAT_VERSION) {
-    error("database file has incorrect version", dbName);
-  }
-
-  if(dbH->headerSize != O2_HEADERSIZE) {
-    error("sizeof(dbTableHeader) unexpected: platform ABI mismatch?", dbName);
-  }
+  dbfid = adb->fd;
+  dbH = adb->header;
 
   CHECKED_MMAP(char *, db, 0, getpagesize());
 

--- a/create.cpp	Mon Nov 24 12:50:38 2008 +0000
+++ b/create.cpp	Mon Nov 24 15:42:15 2008 +0000
@@ -27,8 +27,6 @@
 
 */
 
-int acquire_lock(int, bool);
-
 extern "C" {
   adb_t *audiodb_create(const char *path, unsigned datasize, unsigned ntracks, unsigned datadim) {
     int fd;
@@ -120,7 +118,7 @@
     }
 
     free(header);
-    return audiodb_open(path);
+    return audiodb_open(path, O_RDWR);
 
   error:
     if(header) {

--- a/libtests/0001/prog1.c	Mon Nov 24 12:50:38 2008 +0000
+++ b/libtests/0001/prog1.c	Mon Nov 24 15:42:15 2008 +0000
@@ -33,7 +33,7 @@
     /* create new db */
     //# creation
     //${AUDIODB} -N -d testdb
-    mydbp=audiodb_open(databasename);
+    mydbp=audiodb_open(databasename,O_RDWR);
 
 
     /* open should fail (return NULL), so create a new db */
@@ -69,7 +69,7 @@
 
 /* should pass now - db exists */ 
 //expect_clean_error_exit ${AUDIODB} -N -d testdb
-    mydbp2=audiodb_open(databasename);
+    mydbp2=audiodb_open(databasename, O_RDONLY);
     if (!mydbp2){
        returnval=-1;
     }

--- a/libtests/libtest.mk	Mon Nov 24 12:50:38 2008 +0000
+++ b/libtests/libtest.mk	Mon Nov 24 15:42:15 2008 +0000
@@ -28,7 +28,7 @@
 	-ln -s $< $@
 
 test1: prog1.c ../test_utils_lib.h ../../audioDB_API.h
-	gcc -Wall $(ARCH_FLAGS) -laudioDB -L. -Wl,-rpath,. -o $@ $<
+	gcc -std=c99 -Wall $(ARCH_FLAGS) -laudioDB -L. -Wl,-rpath,. -o $@ $<
 
 clean:
-	-rm $(LIBRARY_FULL) $(LIBRARY_VERS) $(LIBRARY)
\ No newline at end of file
+	-rm $(LIBRARY_FULL) $(LIBRARY_VERS) $(LIBRARY)

--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/open.cpp	Mon Nov 24 15:42:15 2008 +0000
@@ -0,0 +1,76 @@
+#include "audioDB.h"
+extern "C" {
+#include "audioDB_API.h"
+}
+
+bool audiodb_check_header(adb_header_t *header) {
+  /* FIXME: use syslog() or write to stderr or something to give the
+     poor user some diagnostics. */
+  if(header->magic == O2_OLD_MAGIC) {
+    return false;
+  }
+  if(header->magic != O2_MAGIC) {
+    return false;
+  }
+  if(header->version != O2_FORMAT_VERSION) {
+    return false;
+  }
+  if(header->headerSize != O2_HEADERSIZE) {
+    return false;
+  }
+  return true;
+}
+
+adb_t *audiodb_open(const char *path, int flags) {
+  adb_t *adb = 0;
+  int fd = -1;
+
+  flags &= (O_RDONLY|O_RDWR);
+  fd = open(path, flags);
+  if(fd == -1) {
+    goto error;
+  }
+  if(acquire_lock(fd, flags == O_RDWR)) {
+    goto error;
+  }
+
+  adb = (adb_t *) malloc(sizeof(adb_t));
+  if(!adb) {
+    goto error;
+  }
+  adb->fd = fd;
+  adb->flags = flags;
+  adb->path = (char *) malloc(1+strlen(path));
+  if(!(adb->path)) {
+    goto error;
+  }
+  strcpy(adb->path, path);
+
+  adb->header = (adb_header_t *) malloc(sizeof(adb_header_t));
+  if(!(adb->header)) {
+    goto error;
+  }
+  if(read(fd, (char *) adb->header, O2_HEADERSIZE) != O2_HEADERSIZE) {
+    goto error;
+  }
+  if(!audiodb_check_header(adb->header)) {
+    goto error;
+  }
+
+  return adb;
+
+ error:
+  if(adb) {
+    if(adb->header) {
+      free(adb->header);
+    }
+    if(adb->path) {
+      free(adb->path);
+    }
+    free(adb);
+  }
+  if(fd != -1) {
+    close(fd);
+  }
+  return NULL;
+}