Index: trunk/Nebulous-Server/Changes
===================================================================
--- trunk/Nebulous-Server/Changes	(revision 17678)
+++ trunk/Nebulous-Server/Changes	(revision 17680)
@@ -2,4 +2,8 @@
 
 0.10
+    - cleanup database error handling: make sure queries are ->finished before
+      showing an expect, remove DBI->rollback from the error paths of methods
+      that don't insert data
+    - improve getxattr_object() error handling
     - document xattr methods
     - change Nebulous::Server::stat() to not die when passed a non-existant key
Index: trunk/Nebulous-Server/lib/Nebulous/Server.pm
===================================================================
--- trunk/Nebulous-Server/lib/Nebulous/Server.pm	(revision 17678)
+++ trunk/Nebulous-Server/lib/Nebulous/Server.pm	(revision 17680)
@@ -1,5 +1,5 @@
 # Copyright (c) 2004-2008  Joshua Hoblitt
 #
-# $Id: Server.pm,v 1.67 2008-05-13 03:55:08 jhoblitt Exp $
+# $Id: Server.pm,v 1.68 2008-05-14 22:25:00 jhoblitt Exp $
 
 package Nebulous::Server;
@@ -291,4 +291,5 @@
         # if we affected more then one row something very bad has happened.
         unless ($rows == 1) {
+            $query->finish;
             $log->logdie("affected row count is $rows instead of 1");
         }
@@ -361,6 +362,4 @@
             unless ( $rows > 0 ) {
                 $query->finish;
-                $db->rollback;
-                $log->debug("rollback");
                 $log->logdie( "storage object does not exist" );
             }
@@ -444,12 +443,12 @@
     my $read_lock;
     my $write_lock;
-    my $query;
 
     eval {
         {
             # this will set update locks
-            $query = $db->prepare_cached( $sql->get_object_locks );
+            my $query = $db->prepare_cached( $sql->get_object_locks );
             my $rows = $query->execute( $key );
             unless ( $rows == 1 ) {
+                $query->finish;
                 $log->logdie( "storage object does not exist" );
             }
@@ -546,12 +545,12 @@
     my $read_lock;
     my $write_lock;
-    my $query;
 
     eval {
         {
             # this will set update locks
-            $query = $db->prepare_cached( $sql->get_object_locks );
+            my $query = $db->prepare_cached( $sql->get_object_locks );
             my $rows = $query->execute($key);
             unless ($rows == 1) {
+                $query->finish;
                 $log->logdie("storage object does not exist");
             }
@@ -667,4 +666,5 @@
         # name, value, ext_id
         my $rows = $query->execute($name, $value, $key);
+        $query->finish;
 
         # if we affected more then one row something very bad has happened.
@@ -678,4 +678,7 @@
             }
         }
+
+        $db->commit;
+        $log->debug("commit");
     };
     if ($@) {
@@ -685,5 +688,142 @@
     }
 
-    eval { 
+    $log->debug("leaving");
+
+    return 1;
+}
+
+
+sub getxattr_object
+{
+    my $self = shift;
+
+    my ($key, $name) = validate_pos(@_,
+        {
+            type        => SCALAR,
+            callbacks   => {
+                'is valid object key' => sub { $self->_is_valid_object_key($_[0]) },
+            },
+        },
+        {
+            type        => SCALAR,
+        },
+    );
+
+    my $log = $self->log;
+    my $sql = $self->sql;
+    my $db  =$self->db;
+
+    $log->debug("entered - @_");
+
+    # ignore volume
+    $key = parse_neb_key($key);
+
+    my $value;
+    eval {
+        my $query = $db->prepare_cached( $sql->get_object_xattr );
+        # ext_id, name
+        my $rows = $query->execute($key, $name);
+
+        # no rows returned means that the xattr does not exist
+        if ($rows == 0) {
+            $query->finish;
+            $log->logdie( "xattr $key:$name does not exist" );
+        }
+        # if we go more then one row bad something very bad has happened.
+        unless ($rows == 1) {
+            $query->finish;
+            $log->logdie( "affected row count is $rows instead of 1" );
+        }
+
+        my $row = $query->fetchrow_hashref;
+        # XXX: DBI bug? ->finish is needed here even though $query is going out
+        # of scope
+        $query->finish;
+        $value = $row->{ 'value' };
+    };
+    $log->logdie("database error: $@") if $@;
+
+    $log->debug("leaving");
+
+    return $value;
+}
+
+
+sub listxattr_object
+{
+    my $self = shift;
+
+    my ($key) = validate_pos(@_,
+        {
+            type        => SCALAR,
+            callbacks   => {
+                'is valid object key' => sub { $self->_is_valid_object_key($_[0]) },
+            },
+        },
+    );
+
+    my $log = $self->log;
+    my $sql = $self->sql;
+    my $db  =$self->db;
+
+    $log->debug("entered - @_");
+
+    # ignore volume
+    $key = parse_neb_key($key);
+
+    my @xattrs;
+    eval {
+        my $query = $db->prepare_cached( $sql->list_object_xattr );
+        # ext_id
+        my $rows = $query->execute($key);
+
+        while (my $row = $query->fetchrow_hashref) {
+            push @xattrs, $row->{ 'name' };
+        }
+    };
+    $log->logdie("database error: $@") if $@;
+
+    $log->debug("leaving");
+
+    return \@xattrs;
+}
+
+
+sub removexattr_object
+{
+    my $self = shift;
+
+    my ($key, $name) = validate_pos(@_,
+        {
+            type        => SCALAR,
+            callbacks   => {
+                'is valid object key' => sub { $self->_is_valid_object_key($_[0]) },
+            },
+        },
+        {
+            type        => SCALAR,
+        },
+    );
+
+    my $log = $self->log;
+    my $sql = $self->sql;
+    my $db  =$self->db;
+
+    $log->debug("entered - @_");
+
+    # ignore volume
+    $key = parse_neb_key($key);
+
+    eval {
+        my $query = $db->prepare_cached( $sql->remove_object_xattr );
+        # ext_id, name
+        my $rows = $query->execute($key, $name);
+        $query->finish;
+
+        # if we affected more then one row something very bad has happened.
+        unless ($rows == 1) {
+            $log->logdie( "affected row count is $rows instead of 1" );
+        }
+
         $db->commit;
         $log->debug("commit");
@@ -701,159 +841,4 @@
 
 
-sub getxattr_object
-{
-    my $self = shift;
-
-    my ($key, $name) = validate_pos(@_,
-        {
-            type        => SCALAR,
-            callbacks   => {
-                'is valid object key' => sub { $self->_is_valid_object_key($_[0]) },
-            },
-        },
-        {
-            type        => SCALAR,
-        },
-    );
-
-    my $log = $self->log;
-    my $sql = $self->sql;
-    my $db  =$self->db;
-
-    $log->debug("entered - @_");
-
-    # ignore volume
-    $key = parse_neb_key($key);
-
-    my $query;
-    eval {
-        $query = $db->prepare_cached( $sql->get_object_xattr );
-        # ext_id, name
-        my $rows = $query->execute($key, $name);
-
-        # if we affected more then one row something very bad has happened.
-        unless ($rows == 1) {
-            $log->logdie( "affected row count is $rows instead of 1" );
-        }
-    };
-    if ($@) {
-        $db->rollback;
-        $log->debug("rollback");
-        $log->logdie("database error: $@");
-    }
-
-    my $row = $query->fetchrow_hashref;
-    my $value = $row->{ 'value' };
-    $query->finish;
-
-    $log->debug("leaving");
-
-    return $value;
-}
-
-
-sub listxattr_object
-{
-    my $self = shift;
-
-    my ($key) = validate_pos(@_,
-        {
-            type        => SCALAR,
-            callbacks   => {
-                'is valid object key' => sub { $self->_is_valid_object_key($_[0]) },
-            },
-        },
-    );
-
-    my $log = $self->log;
-    my $sql = $self->sql;
-    my $db  =$self->db;
-
-    $log->debug("entered - @_");
-
-    # ignore volume
-    $key = parse_neb_key($key);
-
-    my $query;
-    eval {
-        $query = $db->prepare_cached( $sql->list_object_xattr );
-        # ext_id
-        my $rows = $query->execute($key);
-    };
-    if ($@) {
-        $db->rollback;
-        $log->debug("rollback");
-        $log->logdie("database error: $@");
-    }
-
-    my @xattrs;
-    while (my $row = $query->fetchrow_hashref) {
-        push @xattrs, $row->{ 'name' };
-    }
-
-    $log->debug("leaving");
-
-    return \@xattrs;
-}
-
-
-sub removexattr_object
-{
-    my $self = shift;
-
-    my ($key, $name) = validate_pos(@_,
-        {
-            type        => SCALAR,
-            callbacks   => {
-                'is valid object key' => sub { $self->_is_valid_object_key($_[0]) },
-            },
-        },
-        {
-            type        => SCALAR,
-        },
-    );
-
-    my $log = $self->log;
-    my $sql = $self->sql;
-    my $db  =$self->db;
-
-    $log->debug("entered - @_");
-
-    # ignore volume
-    $key = parse_neb_key($key);
-
-    my $query;
-    eval {
-        $query = $db->prepare_cached( $sql->remove_object_xattr );
-        # ext_id, name
-        my $rows = $query->execute($key, $name);
-
-        # if we affected more then one row something very bad has happened.
-        unless ($rows == 1) {
-            $log->logdie( "affected row count is $rows instead of 1" );
-        }
-    };
-    if ($@) {
-        $db->rollback;
-        $log->debug("rollback");
-        $log->logdie("database error: $@");
-    }
-
-    eval { 
-        $db->commit;
-        $log->debug("commit");
-    };
-    if ($@) {
-        $db->rollback;
-        $log->debug("rollback");
-        $log->logdie("database error: $@");
-    }
-
-    $log->debug("leaving");
-
-    return 1;
-}
-
-
 sub find_objects
 {
@@ -883,17 +868,15 @@
     }
 
-    my $query;
-    eval {
-        $query = $db->prepare_cached( $sql->find_objects );
+    my @keys;
+    eval {
+        my $query = $db->prepare_cached( $sql->find_objects );
         $query->execute( $pattern );
-    };
-    $log->logdie( "database error: $@" ) if $@;
-
-    my @keys;
-
-    while ( my $row = $query->fetchrow_hashref ) {
-        my $key = $row->{ 'ext_id' };
-        push @keys, $key if $key;
-    }
+
+        while ( my $row = $query->fetchrow_hashref ) {
+            my $key = $row->{ 'ext_id' };
+            push @keys, $key if $key;
+        }
+    };
+    $log->logdie("database error: $@") if $@;
 
     $log->debug( "no keys found" ) unless ( scalar @keys );
@@ -945,6 +928,7 @@
     }
 
-    my $query;
-    eval {
+    my @locations;
+    eval {
+        my $query;
         if ($vol_name) {
             $query = $db->prepare_cached( $sql->get_object_instances_by_vol_name );
@@ -964,13 +948,11 @@
             }
         }
+
+        while (my $row = $query->fetchrow_hashref) {
+            my $instance = $row->{ 'uri' };
+            push @locations, $instance if $instance;
+        }
     };
     $log->logdie("database error: $@") if $@;
-
-    my @locations;
-
-    while (my $row = $query->fetchrow_hashref) {
-        my $instance = $row->{ 'uri' };
-        push @locations, $instance if $instance;
-    }
 
     # XXX remove this?
@@ -1103,9 +1085,5 @@
         $query->finish;
     };
-    if ( $@ ) {
-        $db->rollback;
-        $log->debug("rollback");
-        $log->logdie("database error: $@");
-    }
+    $log->logdie("database error: $@") if $@;
 
     $log->debug("leaving");
@@ -1199,9 +1177,5 @@
         $query->finish;
     };
-    if ($@) {
-        $db->rollback;
-        $log->debug("rollback");
-        $log->logdie("database error: $@");
-    }
+    $log->logdie("database error: $@") if $@;
 
     $log->logdie("failed to find a suitable volume" )
@@ -1262,9 +1236,5 @@
         $query->finish;
     };
-    if ($@) {
-        $db->rollback;
-        $log->debug("rollback");
-        $log->logdie( "database error: $@" );
-    }
+    $log->logdie("database error: $@") if $@;
 
     if (defined $vol_id and defined $vol_path) {