Add exportname option to CLI and interop tests

.travis.yml

@@ -11,26 +11,31 @@ env:
   - secure: iS8MtXltyXj/T1EgpZaMillH/SuMAinX7ma5ER1Zt54F9CS3/tH+pG9Yx8uM8sUOZU90xvmN0AZ4HI4sPzquj8UUS+a0Xoj4kj43eGG90bi3nqtdNtopQOZpLFhvm7+tMzRztHLocvlPWLu1l8xHeGycm+J7X1sx+CW+p4uotPE=
   - PINS="nbd:. nbd-lwt-unix:. nbd-tool:."
   - SCRIPT=.travis-docker.sh
+  # Pass through these extra env vars into the Docker container
+  # STRICT=true: Fail the nbd-tool interop tests if the requirements are missing
+  - EXTRA_ENV="STRICT=true"
   matrix:
-  # We need to pass some Travis environment variables to the container to
-  # enable uploading to coveralls and detection of Travis CI.
-  # Also, set TESTS to false to avoid running them twice.
-  # We also have to install the test dependencies because they are not there
-  # when POST_INSTALL_HOOK is run, even if TESTS is set to true.
   - PACKAGE=nbd  DISTRO="debian-unstable" OCAML_VERSION=4.06 \
-    TESTS=false \
-    POST_INSTALL_HOOK="opam install alcotest alcotest-lwt io-page-unix; env TRAVIS=$TRAVIS TRAVIS_JOB_ID=$TRAVIS_JOB_ID bash -ex coverage.sh"
   - PACKAGE=nbd  DISTRO="debian-unstable" OCAML_VERSION=4.07
   # Upload docs for both nbd and nbd-lwt-unix
   - PACKAGE=nbd-lwt-unix  DISTRO="debian-unstable" OCAML_VERSION=4.06 SCRIPT=.travis-docker-docgen.sh
   - PACKAGE=nbd-lwt-unix  DISTRO="debian-unstable" OCAML_VERSION=4.07
-  - PACKAGE=nbd-tool  DISTRO="debian-unstable" OCAML_VERSION=4.06
-  - PACKAGE=nbd-tool  DISTRO="debian-unstable" OCAML_VERSION=4.07
+  # We set TESTS to false to avoid running them twice.
+  # We install the packages used for interop tests with nbd-tool in
+  # PRE_INSTALL_HOOK.
+  # We also have to install the opam test dependencies because they are not there
+  # when POST_INSTALL_HOOK is run, even if TESTS is set to true.
+  # And we hvae to pass some Travis environment variables to the container to
+  # enable uploading to coveralls and detection of Travis CI.
+  - PACKAGE=nbd-tool DISTRO="debian-unstable" PRE_INSTALL_HOOK="sudo apt-get install -y qemu-utils nbd-client" OCAML_VERSION=4.06 \
+    TESTS=false \
+    POST_INSTALL_HOOK="opam install alcotest alcotest-lwt io-page-unix; env TRAVIS=$TRAVIS TRAVIS_JOB_ID=$TRAVIS_JOB_ID bash -ex coverage.sh"
+  - PACKAGE=nbd-tool DISTRO="debian-unstable" PRE_INSTALL_HOOK="sudo apt-get install -y qemu-utils nbd-client" OCAML_VERSION=4.07
 matrix:
   fast_finish: true
   allow_failures:
     - env: PACKAGE=nbd  DISTRO="debian-unstable" OCAML_VERSION=4.07
     - env: PACKAGE=nbd-lwt-unix  DISTRO="debian-unstable" OCAML_VERSION=4.07
-    - env: PACKAGE=nbd-tool  DISTRO="debian-unstable" OCAML_VERSION=4.07
+    - env: PACKAGE=nbd-tool DISTRO="debian-unstable" PRE_INSTALL_HOOK="sudo apt-get install -y qemu-utils nbd-client" OCAML_VERSION=4.07
 branches:
   only: master

Makefile

@@ -18,7 +18,7 @@ clean:
 test:
 	dune runtest
 
-# requires sudo access, nbd-client, and hdparm
+# requires qemu-img
 benchmark: build
 	./benchmark.sh
 

benchmark.sh

@@ -1,32 +1,18 @@
 #!/bin/bash
 
-# Run this script to benchmark the NBD server using nbd-client and hdparm.
+# Run this script to benchmark the NBD server using qemu-img.
 
 set -eux
 
 dd if=/dev/zero of=/tmp/test bs=1M count=100
-_build/default/cli/main.exe serve --no-tls /tmp/test &
+_build/default/cli/main.exe serve --exportname test --no-tls /tmp/test &
 SERVER_PROCESS=$!
-echo $SERVER_PROCESS
+# Wait for the server to start the main loop
+sleep 0.1
 
-function stop_server {
-  kill $(jobs -p)
+stop_server() {
+  kill -9 $SERVER_PROCESS
 }
 trap stop_server EXIT
 
-
-sudo modprobe nbd
-
-sudo nbd-client -N test localhost /dev/nbd0
-
-function stop_client {
-  sudo nbd-client -d /dev/nbd0
-  stop_server
-}
-trap stop_client EXIT
-
-sudo hdparm -t /dev/nbd0
-sudo hdparm -t /dev/nbd0
-sudo hdparm -t /dev/nbd0
-sudo hdparm -t /dev/nbd0
-sudo hdparm -t /dev/nbd0
+qemu-img bench 'nbd:0.0.0.0:10809:exportname=test'

cli/main.ml

@@ -173,17 +173,30 @@ module Impl = struct
 
   let ignore_exn t () = Lwt.catch t (fun _ -> Lwt.return_unit)
 
-  let serve _common filename port certfile ciphersuites no_tls =
+  let serve _common filename port exportname certfile ciphersuites no_tls =
     let tls_role = init_tls_get_server_ctx ~certfile ~ciphersuites no_tls in
     let filename = require "filename" filename in
+    let validate ~client_exportname =
+      match exportname with
+      | Some exportname when exportname <> client_exportname ->
+        Lwt.fail_with
+          (Printf.sprintf "Client requested invalid exportname %s, name of the export is %s" client_exportname exportname)
+      | _ -> Lwt.return_unit
+    in
     let handle_connection fd =
       Lwt.finalize
         (fun () ->
            Nbd_lwt_unix.with_channel fd tls_role
              (fun clearchan ->
+                let offer = match exportname with
+                  | None -> None
+                  | Some exportname -> Some [exportname]
+                in
                 Server.with_connection
+                  ?offer
                   clearchan
-                  (fun _exportname svr ->
+                  (fun client_exportname svr ->
+                     validate ~client_exportname >>= fun () ->
                      Nbd_lwt_unix.with_block filename
                        (Server.serve svr ~read_only:false (module Block))
                   )
@@ -286,10 +299,19 @@ let serve_cmd =
   let port =
     let doc = "Local port to listen for connections on" in
     Arg.(value & opt int 10809 & info [ "port" ] ~doc) in
+  let exportname =
+    let doc = {|Export name to use when serving the file. If specified, clients
+                will be able to list this export, and only this export name
+                will be accepted. If unspecified, listing the exports will not
+                be allowed, and all export names will be accepted when
+                connecting.|}
+    in
+    Arg.(value & opt (some string) None & info ["exportname"] ~doc)
+  in
   let no_tls =
     let doc = "Use NOTLS mode (refusing TLS) instead of the default FORCEDTLS." in
     Arg.(value & flag & info ["no-tls"] ~doc) in
-  Term.(ret(pure Impl.serve $ common_options_t $ filename $ port $ certfile $ ciphersuites $ no_tls)),
+  Term.(ret(pure Impl.serve $ common_options_t $ filename $ port $ exportname $ certfile $ ciphersuites $ no_tls)),
   Term.info "serve" ~sdocs:_common_options ~doc ~man
 
 let mirror_cmd =

cli_test/dune

@@ -0,0 +1,20 @@
+(executable
+ (name suite)
+ (libraries
+  alcotest
+  alcotest-lwt
+  cmdliner
+ )
+)
+
+(alias
+ (name runtest)
+ (package nbd-tool)
+ (deps
+  (:suite suite.exe)
+  (:cli ../cli/main.exe)
+  ./test-qemu.sh
+  ./test-nbd-client.sh
+ )
+ (action (run %{suite} --cli %{cli}))
+)

cli_test/suite.ml

@@ -0,0 +1,42 @@
+
+let with_command ~command ~strict test =
+  match Sys.command ("command -v " ^ command) with
+  | 0 -> test ()
+  | _ ->
+    if strict then failwith ("Command " ^ command ^ " not present")
+    else Printf.printf "!!! Skipping test because command %s is not available" command
+
+let script name ~requires (cli, strict) =
+  with_command
+    ~command:requires
+    ~strict
+    (fun () ->
+       Alcotest.(check int)
+         name
+         0
+         (Sys.command (name ^ " " ^ cli))
+    )
+
+let cli =
+  let doc = "Path to nbd CLI should be first command-line argument" in
+  Cmdliner.Arg.(required & opt ~vopt:None (some string) None & info ["cli"] ~docv:"CLI" ~doc)
+
+let strict =
+  let doc = {|If present, the test will fail when the required program is not
+              installed. Otherwise the test will simply be skipped.|}
+  in
+  Cmdliner.Arg.(value & flag & info ["strict"] ~env:(env_var "STRICT") ~doc)
+
+let opts = Cmdliner.Term.(const (fun cli strict -> (cli, strict)) $ cli $ strict)
+
+let () =
+  Alcotest.run_with_args
+    "Nbd CLI interoperability tests"
+    opts
+    [ ("compatibility with qemu-img",
+       ["data copying", `Slow, script "./test-qemu.sh" ~requires:"qemu-img"]
+      )
+    ; ("compatibility with nbd-client",
+       ["listing exports", `Slow, script "./test-nbd-client.sh" ~requires:"nbd-client"]
+      )
+    ]

cli_test/test-nbd-client.sh

@@ -0,0 +1,26 @@
+#!/bin/sh
+
+set -eux
+
+CLI=$1
+
+SCRATCH=$(mktemp -d)
+EXPORT=$SCRATCH/test
+
+# Create a test file
+dd if=/dev/urandom of="$EXPORT" bs=1M count=40
+
+# Serve it
+$CLI serve --exportname test --no-tls "$EXPORT" &
+SERVER=$!
+# Wait for the server to start the main loop
+sleep 0.1
+
+stop_server() {
+  kill -9 $SERVER
+}
+trap stop_server EXIT
+
+# Try listing the exports
+# This will fail if the server does not implement NBD_OPT_LIST correctly
+nbd-client -l 0.0.0.0

cli_test/test-qemu.sh

@@ -0,0 +1,29 @@
+#!/bin/sh
+
+set -eux
+
+CLI=$1
+
+SCRATCH=$(mktemp -d)
+EXPORT=$SCRATCH/test
+OUTPUT=$SCRATCH/out
+
+# Create a test file
+dd if=/dev/urandom of="$EXPORT" bs=1M count=40
+
+# Serve it
+$CLI serve --exportname test --no-tls "$EXPORT" &
+SERVER=$!
+# Wait for the server to start the main loop
+sleep 0.1
+
+stop_server() {
+  kill -9 $SERVER
+}
+trap stop_server EXIT
+
+# Download it as raw from the server
+qemu-img convert 'nbd:0.0.0.0:10809:exportname=test' -O raw "$OUTPUT"
+
+# Check that the two files are the same
+cmp --silent "$EXPORT" "$OUTPUT"

lib_test/dune

@@ -1,4 +1,4 @@
-(executable
+(test
  (name suite)
  (libraries
   alcotest
@@ -8,8 +8,3 @@
   lwt_log
   nbd)
 )
-
-(alias
- (name   runtest)
- (deps   (:suite suite.exe))
- (action (run %{suite})))

nbd-tool.opam

@@ -6,8 +6,11 @@ homepage: "https://github.com/xapi-project/nbd"
 dev-repo: "https://github.com/xapi-project/nbd.git"
 bug-reports: "https://github.com/xapi-project/nbd/issues"
 build: ["dune" "build" "-p" name "-j" jobs]
+build-test: ["dune" "runtest" "-p" name]
 depends: [
   "dune" {build}
+  "alcotest" {test}
+  "alcotest-lwt" {test}
   "cmdliner"
   "lwt" {>= "2.7.0"}
   "lwt_log"

nbd.opam

@@ -14,6 +14,7 @@ depends: [
   "dune" {build}
   "alcotest" {test}
   "alcotest-lwt" {test}
+  "io-page-unix" {test}
   "cstruct" {>= "3.1.0"}
   "io-page"
   "mirage-block-lwt"