Merge branch '36-breaks-when-trying-to-install-debian-from-cd' into 'develop'

Resolve "breaks when trying to install debian from CD"

Closes #36

See merge request open-source/flexnbd-c!40

debian/changelog

@@ -10,6 +10,10 @@ flexnbd (0.2.0) UNRELEASED; urgency=medium
 
   [ Patrick J Cherry ]
   * Added FLUSH and FUA support (!38) 
+  * Server returns ENOSPC in response to writes beyond the end of the
+    filesystem, and EINVAL to unknown commands. (#36, !40)
+  * Proxy passes all NBD protocol errors through to the client instead of
+    disconnecting and retrying (#36, !40)
 
  -- James Carter <james.carter@bytemark.co.uk>  Thu, 11 Jan 2018 10:05:35 +0000
 

src/proxy/proxy.c

@@ -618,11 +618,6 @@ int proxy_read_from_upstream( struct proxier* proxy, int state )
 			goto disconnect;
 		}
 
-		if ( reply->error != 0 ) {
-			warn( "NBD error returned from upstream: %"PRIu32, reply->error );
-			goto disconnect;
-		}
-
 		if ( proxy->req_hdr.type == REQUEST_READ ) {
 			/* Get the read reply data too. */
 			proxy->rsp.size += proxy->req_hdr.len;

src/server/client.c

@@ -391,7 +391,9 @@ int client_request_needs_reply( struct client * client,
 		request.type, request.flags, request.from, request.len, request.handle
 	);
 
-	/* check it's not out of range */
+	/* check it's not out of range. NBD protocol requires ENOSPC to be
+	 * returned in this instance 
+	 */
 	if ( request.from+request.len > client->serve->size) {
 		warn("write request %"PRIu64"+%"PRIu32" out of range",
 		  request.from, request.len
@@ -399,7 +401,7 @@ int client_request_needs_reply( struct client * client,
 		if ( request.type == REQUEST_WRITE ) {
 			client_flush( client, request.len );
 		}
-		client_write_reply( client, &request, EPERM ); /* TODO: Change to ERANGE ? */
+		client_write_reply( client, &request, ENOSPC );
 		client->disconnect = 0;
 		return 0;
 	}
@@ -418,7 +420,12 @@ int client_request_needs_reply( struct client * client,
 	case REQUEST_FLUSH:
 		break;
 	default:
-		fatal("Unknown request 0x%08X", request.type);
+		/* NBD prototcol says servers SHOULD return EINVAL to unknown
+		 * commands */
+		warn("Unknown request 0x%08X", request.type);
+		client_write_reply( client, &request, EINVAL );
+		client->disconnect = 0;
+		return 0;
 	}
 	return 1;
 }

tests/acceptance/proxy_tests.rb

@@ -72,6 +72,20 @@ module ProxyTests
     end
   end
 
+  def test_write_request_past_end_of_disc_returns_to_client
+    with_proxied_client do |client|
+      n = 1000
+      offset = n * 4096
+      client.write(offset, b * 4096)
+      rsp = client.read_response
+
+      assert_equal FlexNBD::REPLY_MAGIC, rsp[:magic]
+      assert_equal 'myhandle', rsp[:handle]
+      # NBD protocol say ENOSPC (28) in this situation
+      assert_equal 28, rsp[:error]
+    end
+  end
+
   def make_fake_server
     server = FlexNBD::FakeDest.new(@env.ip, @env.port1)
     @server_up = true

tests/acceptance/test_serve_mode.rb

@@ -112,7 +112,8 @@ class TestServeMode < Test::Unit::TestCase
 
       assert_equal FlexNBD::REPLY_MAGIC, rsp[:magic]
       assert_equal 'myhandle', rsp[:handle]
-      assert rsp[:error] != 0, "Server sent success reply back: #{rsp[:error]}"
+      # NBD protocol suggests ENOSPC (28) is returned
+      assert_equal 28, rsp[:error], 'Server sent incorrect response'
 
       # Ensure we're not disconnected by sending a request. We don't care about
       # whether the reply is good or not, here.
@@ -129,7 +130,26 @@ class TestServeMode < Test::Unit::TestCase
 
       assert_equal FlexNBD::REPLY_MAGIC, rsp[:magic]
       assert_equal 'myhandle', rsp[:handle]
-      assert rsp[:error] != 0, "Server sent success reply back: #{rsp[:error]}"
+      # NBD protocol suggests ENOSPC (28) is returned
+      assert_equal 28, rsp[:error], 'Server sent incorrect response'
+
+      # Ensure we're not disconnected by sending a request. We don't care about
+      # whether the reply is good or not, here.
+      client.write(0, "\x00" * @env.file1.size)
+      rsp = client.read_response
+      assert_equal FlexNBD::REPLY_MAGIC, rsp[:magic]
+    end
+  end
+
+  def test_unknown_command_receives_error_response
+    connect_to_server do |client|
+      client.send_request(123)
+      rsp = client.read_response
+
+      assert_equal FlexNBD::REPLY_MAGIC, rsp[:magic]
+      assert_equal 'myhandle', rsp[:handle]
+      # NBD protocol suggests EINVAL (22) is returned
+      assert_equal 22, rsp[:error], 'Server sent incorrect response'
 
       # Ensure we're not disconnected by sending a request. We don't care about
       # whether the reply is good or not, here.