Index: /branches/eam_branches/ipp-20191011/Nebulous-Server/lib/Nebulous/Server.pm
===================================================================
--- /branches/eam_branches/ipp-20191011/Nebulous-Server/lib/Nebulous/Server.pm	(revision 41013)
+++ /branches/eam_branches/ipp-20191011/Nebulous-Server/lib/Nebulous/Server.pm	(revision 41014)
@@ -12,4 +12,6 @@
 
 use base qw( Class::Accessor::Fast );
+
+use Carp;
 
 use Cache::Memcached;
@@ -723,11 +725,4 @@
         {
             type        => SCALAR|UNDEF,
-#            callbacks   => {
-#                # check that the volume name requested is valid
-#                'is valid volume name' => sub {
-#                    return 1 if not defined $_[0];
-#                    $self->_is_valid_volume_name($_[0])
-#                },
-#            },
             optional    => 1,
         },
@@ -737,15 +732,20 @@
 
     # if a volume name is explicity specified then we should make the
-    # replication onto that volume (even if there is alread an instance on that
+    # replication onto that volume (even if there is already an instance on that
     # volume) if at all possible and throw an error if we can not.
+
     # if a volume name IS NOT specified then we should make the replication
     # onto any (hopefully the best) volume that DOES NOT already have an
-    # instance on it.  If all avilable volume already have an instance on them
+    # instance on it.  If all available volumes already have an instance on them
     # then we should throw an error 
+
     # volume names implied as part of the key are *IGNORED* as the source and
     # *SHOULD NOT* be used as the destination either
 
     eval {
-        $key = parse_neb_key($key);
+	# EAM 2019.10.31 : if I supply dest_vol_name here, then
+	# $key->volume will point at that volume and the section below
+	# will correctly be testing the validity of dest_vol_name
+        $key = parse_neb_key($key, $dest_vol_name);
     };
     $log->logdie("$@") if $@;
@@ -753,6 +753,13 @@
     my $db  = $self->db($key);
 
+    ## Old comment:
     # puke if the source volume is bogus, we may want to actually use this as
     # the instance to be copied later
+
+    # EAM 2019.10.31 : in the past, the key-implied volume was
+    # supplied here, which was inconsistent since above it says the
+    # key-implied volume is ignored. By supplying $dest_vol_name to
+    # parse_neb_key above, the key->volume now refers to the
+    # dest_volume
     if (defined $key->volume and not $self->_is_valid_volume_name($key, $key->volume)) {
         unless ($key->hard_volume) {
@@ -762,9 +769,11 @@
         }
     }
+
     # puke if the destination volume is bogus
-    if (defined $dest_vol_name
-        and not $self->_is_valid_volume_name($key, $dest_vol_name)) {
-           $log->logdie($key->volume . " is not a valid volume name");
-    }
+    ## XXX if I supply dest_vol_name to parse_neb_key above, this section below is redundant with the above
+    ## if (defined $dest_vol_name
+    ##     and not $self->_is_valid_volume_name($key, $dest_vol_name)) {
+    ##        $log->logdie($key->volume . " is not a valid volume name");
+    ## }
 
     $get_storage_volume_calls = 0;
@@ -772,10 +781,14 @@
     my ($vol_id, $vol_host, $vol_path, $vol_xattr);
     if (defined $dest_vol_name) {
+	# if we supplied dest_vol_name, then the call to parse_neb_key above
+	# will have set $key->volume to that value and hard_volume to match
         ($vol_id, $vol_host, $vol_path, $vol_xattr)
-            = $self->_get_storage_volume($key, $dest_vol_name);
+            = $self->_get_storage_volume($key, $key->volume, $key->hard_volume);
     } else {
         ($vol_id, $vol_host, $vol_path, $vol_xattr)
             = $self->_get_replication_volume($key);
     }
+
+    # print "selected target: $vol_id, $vol_host\n";
 
     my $uri;
@@ -1789,11 +1802,4 @@
         {
             type        => SCALAR|UNDEF,
-#            callbacks   => {
-#                # check that the volume name requested is valid
-#                'is valid volume name' => sub {
-#                    return 1 if not defined $_[0];
-#                    $self->_is_valid_volume_name($_[0])
-#                },
-#            },
             optional    => 1,
         },
@@ -1807,8 +1813,6 @@
     my $sql = $self->sql;
 
-#    unless ($key) {
-#        $log->warn("key was undefined after validate_pos(), trying again...");
-#        return $self->find_instances(@_);
-#    }
+    # default value for find_invalid is false
+    if (not defined $find_invalid) { $find_invalid = 0; }
 
     # vol_name overrides the key implied volume
@@ -1848,5 +1852,5 @@
 	    my $rows;
             # ext_id, available
-	    if (defined($find_invalid)) {
+	    if (defined($find_invalid) and $find_invalid) {
 		# XXX returns instances which are NOT available
 		$rows = $query->execute($key->path, 0);
@@ -1903,11 +1907,4 @@
         {
             type        => SCALAR|UNDEF,
-#            callbacks   => {
-#                # check that the volume name requested is valid
-#                'is valid volume name' => sub {
-#                    return 1 if not defined $_[0];
-#                    $self->_is_valid_volume_name($_[0])
-#                },
-#            },
             optional    => 1,
         },
@@ -1920,8 +1917,5 @@
     my $sql = $self->sql;
 
-#    unless ($key) {
-#        $log->warn("key was undefined after validate_pos(), trying again...");
-#        return $self->find_instances(@_);
-#    }
+    if (not defined $find_invalid) { $find_invalid = 0; }
 
     # vol_name overrides the key implied volume
@@ -1991,6 +1985,11 @@
 	    # host_vol_id host_cab_id host_site_id ext_id available
 
-	    ### XXX EAM if find_invalid is true, the last arguement here should be 0
-            my $rows = $query->execute($h_vol_id, $h_cab_id, $h_site_id, $key->path, 1);
+	    my $rows;
+	    if (defined($find_invalid) and $find_invalid) {
+		# returns instances which are NOT available (volume not available)
+		$rows = $query->execute($h_vol_id, $h_cab_id, $h_site_id, $key->path, 0);
+	    } else {
+		$rows = $query->execute($h_vol_id, $h_cab_id, $h_site_id, $key->path, 1);
+	    }		
             unless ($rows > 0) {
                 $query->finish;
@@ -1999,11 +1998,11 @@
         } else {
             $query = $db->prepare_cached( $sql->get_object_instances );
+            # ext_id, available
+
 	    my $rows;
-            # ext_id, available
-	    if (defined($find_invalid)) {
+	    if (defined($find_invalid) and $find_invalid) {
 		# returns instances which are NOT available (volume not available)
 		$rows = $query->execute($key->path, 0);
-	    }
-	    else {
+	    } else {
 		$rows = $query->execute($key->path, 1);
 	    }
@@ -2020,4 +2019,6 @@
             my $instance = $row->{ 'uri' };
             push @locations, $instance if $instance;
+
+	    # Carp::carp ("instance 2: $row->{ 'uri' }, $row->{ 'vol_id' }, $row->{ 'cab_id' }, $row->{ 'vol_idx' }\n");
         }
     };
@@ -2027,5 +2028,5 @@
         if (defined $vol_name and not defined $key->hard_volume) {
             $log->debug("retrying with 'any' volume");
-            return $self->find_instances($key->path, 'any');
+            return $self->find_instances($key->path, 'any', $find_invalid);
         }
         $log->logdie("database error: $@");
@@ -2463,5 +2464,5 @@
             # %free, name, avaiable, allocate
             $rows = $query->execute($max_used_space, $name, 1, 1);
-            # XXX destinguish between non-existant and unavailable
+            # XXX distinguish between non-existant and unavailable
             unless ($rows > 0) {
                 $query->finish;
@@ -2572,5 +2573,5 @@
 
         $rows = $query->execute($key->path, $max_used_space, 1, 1, $forbidden_cabinet, $forbidden_site, $topfew_count);
-        # XXX destinguish between non-existant and unavailable
+        # XXX distinguish between non-existant and unavailable
         unless ($rows > 0) {
             $query->finish;