From 8beb3f0af6633fdb67dc99fabcef00f6cd49cbb7 Mon Sep 17 00:00:00 2001 From: Patrick J Cherry Date: Thu, 8 Feb 2018 13:19:51 +0000 Subject: [PATCH 1/2] Allow proxy to pass NBD protocol errors downstream; server returns EINVAL/ENOSPC appropriately Previously the proxy would just disconnect when it saw an NBD protocol error, and retry the operation it was in the middle of. Additionally, the server needs to return the correct error types when this happens. --- src/proxy/proxy.c | 5 ----- src/server/client.c | 13 ++++++++++--- tests/acceptance/proxy_tests.rb | 14 ++++++++++++++ tests/acceptance/test_serve_mode.rb | 24 ++++++++++++++++++++++-- 4 files changed, 46 insertions(+), 10 deletions(-) diff --git a/src/proxy/proxy.c b/src/proxy/proxy.c index 8283d8c..b323c68 100644 --- a/src/proxy/proxy.c +++ b/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; diff --git a/src/server/client.c b/src/server/client.c index 3a72cb9..8ca4d2a 100644 --- a/src/server/client.c +++ b/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; } diff --git a/tests/acceptance/proxy_tests.rb b/tests/acceptance/proxy_tests.rb index c2623d1..65fe38a 100644 --- a/tests/acceptance/proxy_tests.rb +++ b/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 diff --git a/tests/acceptance/test_serve_mode.rb b/tests/acceptance/test_serve_mode.rb index 5aa8912..4f9a77e 100644 --- a/tests/acceptance/test_serve_mode.rb +++ b/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. From 5e9dbbd626f3f34ba3effc25970a642b353d9974 Mon Sep 17 00:00:00 2001 From: Patrick J Cherry Date: Thu, 8 Feb 2018 13:32:10 +0000 Subject: [PATCH 2/2] Updated changelgo --- debian/changelog | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/debian/changelog b/debian/changelog index b05e74d..f25f071 100644 --- a/debian/changelog +++ b/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 Thu, 11 Jan 2018 10:05:35 +0000