Nbd needs changes to support lwt after the split between Lwt and Lwt_log. The new recommended way is to use log and fmt, lwt_log will be deprecated soon

It expects one more 32 bit int than what the protocol describes. This is because it seems to parse the length twice in the code.

To prevent such issues, we could add a new kind of unit test, which connects the server and the client together. We could also add an integration-style test, provided we can run the nbd-client / nbd-server programs on Travis.

• When this is fixed, some of the test cases marked with TODO may need to be updated to remove the workaround that was added due to this bug.

The NBD protocol says:

bit 8, NBD_FLAG_CAN_MULTI_CONN: Indicates that the server operates
entirely without cache, or that the cache it uses is shared among all
connections to the given device. In particular, if this flag is
present, then the effects of NBD_CMD_FLUSH and NBD_CMD_FLAG_FUA
MUST be visible across all connections when the server sends its reply
to that command to the client. In the absense of this flag, clients
SHOULD NOT multiplex their commands over more than one connection to
the export.

We can and should do this, because we do not do caching at the moment, and this flag might enable the parallel connections feature in nbd-client.

I repeated the following commands:

    c = new_nbd_client("localhost")
    c.close()

first with 0, then 1s, and finally with a 10s delay between each iteration. In each case, as clients kept connecting and disconnecting, the memory usage of the NBD server grew linearly. In each case, after about 500 iterations, when the server was using about 230Mb of memory, it crashed:

nbd-tool: internal error, uncaught exception:
          Unix.Unix_error(Unix.EINVAL, "select", "")

I observed the same behaviour both with a custom Python client, (which doesn't close the TCP session during c.close(), because the server has to do it according to the NBD protocol), and versions 3.8 and 3.13 of the standard nbd-client. In the case of nbd-client, I repeated these commands:

sudo nbd-client localhost /dev/nbd2 -N ""
nbd-client -d /dev/nbd2

The reason for this issue might be that the NBD server does not handle NBD_CMD_DISC properly, it returns EINVAL.

I observed this issue with NBD server version: 2.1.3, but it seems that 3.0.0 is also affected, but it behaves a bit differently. When its memory usage reaches 90Mb, it decreases to about 14Mb again. However, it still crashes after about 500 connections, but with a different error:

nbd-tool: internal error, uncaught exception:
          Unix.Unix_error(Unix.EMFILE, "accept", "")

Quoting from the NBD protocol,

Client flags

This field of 32 bits is sent after initial connection and after
receiving the handshake flags from the server.

• [irrelevant part snipped]
• bit 1, NBD_FLAG_C_NO_ZEROES; MUST NOT be set if the server did not set NBD_FLAG_NO_ZEROES. If set, the server MUST NOT send the 124 bytes of zeroes at the end of the negotiation.

Clients MUST NOT set any other flags; the server MUST drop the TCP connection if the client sets an unknown flag, or a flag that does not match something advertised by the server.

The server fails to do the action I have emphasised above. It does not set NBD_FLAG_NO_ZEROES in the intial handshake flags it sends, but then it makes no decisions that depend on whether the client set NBD_FLAG_C_NO_ZEROES: instead the server carries on regardless and sends the zeroes.

If we were to add support for this flag and feature, then Protocol.Diskinfo.t should omit the padding if the client flag is set.

Unlike the handler of the Read command in loop in lib/server.ml, the code that handles the Write command does not call loop recursively to continue processing requests. As a result, the server gets stuck after the write command, and the client will hang if it sends any further commands. Both version v2.1.3 that we currently use in xs-opam, and the most recent v3.0.0 version are affected.

This issue can be fixed by replacing this line:

            else ok t handle None in

with this:

            else ok t handle None >>= fun () -> loop () in

(Maybe we should also call the loop recursively in the invalid request case, it depends on what the NBD protocol dictates.)

We query the block size with mirage_block, and enforce it by returning
EINVAL if not aligned: (Block.get_info b).sector_size, both in 3.0.0 and
2.3.1. It seems to get the block size using the BLKSSZGET ioctl.

The NBD protocol says the following about the block size:

A server with block size constraints other than the default SHOULD
advertise the block size constraints during handshake phase via
NBD_INFO_BLOCK_SIZE in response to NBD_OPT_INFO or NBD_OPT_GO,
and MUST do so unless it has agreed on block size constraints via out
of band means.

If block size constraints have not been advertised or agreed on externally,
then a client SHOULD assume a default minimum block size of 1, a preferred
block size of 2^12 (4,096), and a maximum block size of the smaller of
the export size or 0xffffffff (effectively unlimited).

A server that
wants to enforce block sizes other than the defaults specified here
MAY refuse to go into transmission phase with a client that uses
NBD_OPT_EXPORT_NAME (via a hard disconnect) or which fails to use
NBD_INFO_BLOCK_SIZE with NBD_OPT_GO (where the server uses
NBD_REP_ERR_BLOCK_SIZE_REQD), although a server SHOULD permit such
clients if block size constraints are the default or can be agreed on
externally. When allowing such clients, the server MUST cleanly error
commands that fall outside block size constraints without corrupting
data; even so, this may limit interoperability.

The server SHOULD report an EINVAL error if
the client's request is not aligned to advertised minimum block size
boundaries, or is larger than the advertised maximum block size,
although the server MAY instead initiate a hard disconnect if a large
length could be deemed as a denial of service attack.

In the case of xapi-nbd, we don't have to worry about denial of service attacks, because the
client has to be authenticated first. For the best compatibility, we
could advertise the minimum block size in the docs to handle clients
that do not support NBD_OPT_GO. However, this block size isn't constant,
because we get it from the mirage library - so we MUST use NBD_OPT_GO to
communicate this runtime minimum block size.

This NBD server doesn't support NBD_OPT_GO yet.

Since it does not return a value representing the NBD session, Client.list should terminate the session itself. However, it does that using a hard disconnect, even when that is forbidden.
The NBD protocol documentation says:

A party that is mandated by this document to terminate the
session MUST initiate a hard disconnect if it is not possible
to use a soft disconnect. Such circumstances include: where
that party is the server and it cannot return an error
(e.g. after an NBD_OPT_EXPORT_NAME it cannot satisfy),
and where that party is the client following a failed TLS
negotiation.

A party MUST NOT initiate a hard disconnect save where set out
in this section. Therefore, unless a client's situation falls
within the provisions of the previous paragraph or the
client detects a breach of a mandatory condition, it MUST NOT
use a hard disconnect, and hence its only option to terminate
the session is via a soft disconnect.

The Client.list function MUST send NBD_OPT_ABORT if the list or a suitable error is returned from the server.

• When this is fixed, some of the test cases marked with TODO may need to be updated to remove the workaround that was added due to this bug.

It's partly because of #139 - we do not even parse the command flags.
To fix this, we'd have to fix #139 to correctly parse the command flags, and the return EINVAL if we receive any unsupported ones.

This email says that well-behaved servers should respond with einval to unrecognised flags: https://lists.debian.org/nbd/2019/03/msg00004.html

Hi,
I have NBD Server 3.20 installed on RHEL7 Operating system and configured. While adding new exports under NBD server config, i need to reload new configuration for the exports. While sending SIGHUP signal via OS process to the root PID, it makes NBD-SERVER process dead.
Later, i tried with a few more test attempts with the same SIGHUP signal after NBD-SERVER service restart & re-adding exports under config. Sometimes, it works for a few iterations but later it fails. Currently, i have only 5 exported devices in nbd server config, planned to scale up with multiple exports as per needs.

Please assist or advise any feedback..

To make it easier to verify that we obey the NBD protocol. If we just have these random numbers, they can easily be off-by-one for example.

We need to either

1. include the Result module in the package
2. switch to using a polymorphic variant

I prefer (2) since it avoids polluting the namespace with "Result" -- this tends to cause libraries to clash unnecessarily. I'll take a look.

As ocsigen/lwt#468 (comment) said, Lwt_log might be deprecated in the future.

See ocaml/opam-repository#15541 for details
cc @psafont @lippirk @edwintorok @lindig @mseri

This is probably not a big issue in practice, because when the dispatcher background thread tries to read the next message, it will just fail and then stop. This exception in the background thread probably will not be noticed by the application using the library.
But probably it's better in general to avoid exceptions happening during normal use cases, if possible.

The only possible issue that could happen is that if reads / writes don't fail soon, then these dispatcher background threads will keep accumulating as the client connects & disconnects to/from servers.

• A small file whose size is below the block size appears as a disk of size 0.
• Probably if we have a larger file whose end is not aligned, we cannot read it the extra bytes (this needs to be confirmed).

One way to fix this is to just exit with an error if the user tries to serve these files.

The NBD protocol says:

Request message

The request message, sent by the client, looks as follows:

C: 32 bits, 0x25609513, magic (NBD_REQUEST_MAGIC)
C: 16 bits, command flags
C: 16 bits, type
C: 64 bits, handle
C: 64 bits, offset (unsigned)
C: 32 bits, length (unsigned)
C: (length bytes of data if the request is of type NBD_CMD_WRITE)

There is a 16-bit flags and a 16-bit type field
But we have a single 32-bit type field in our code:

 820   [%%cstruct                                                                                                                                                                
 821     type t = {                                                                                                                                                              
 822       magic:  uint32_t;                                                                                                                                                     
 823       ty:     uint32_t;                                                                                                                                                     
 824       handle: uint64_t;                                                                                                                                                     
 825       from:   uint64_t;                                                                                                                                                     
 826       len:    uint32_t;                                                                                                                                                     
 827     } [@@big_endian]                                                                                                                                                        
 828   ] 

This is not an issue currently, because we do not support any command flags.

Add interop tests with nbd-client and qemu-img

We will typically want to run the mirroring tool in the background. We will need:

• a --daemon option
• a control Unix domain socket
• a simple protocol to support querying the mirror state and shutting down the mirror

I've found this issue by trying to connect from qemu (I've specified a list of offers in the cli code), listing the exports with nbd-client doesn't work either:

~/src $ qemu-img convert nbd:0.0.0.0:10809:exportname=test -O qcow2 /tmp/out                                                                                                     
qemu-img: Could not open 'nbd:0.0.0.0:10809:exportname=test': incorrect option name length 
~/src $ nbd-client -l localhost
Negotiation: ..

E: export name on server too long
~/src $ nbd-client -l 0.0.0.0
Negotiation: ..

E: export name on server too long

The TCP stream:

    00000000  4e 42 44 4d 41 47 49 43  49 48 41 56 45 4f 50 54 NBDMAGIC IHAVEOPT
    00000010  00 01                                            ..
00000000  00 00 00 01                                      ....
00000004  49 48 41 56 45 4f 50 54  00 00 00 08 00 00 00 00 IHAVEOPT ........
    00000012  00 03 e8 89 04 55 65 a9  00 00 00 08 80 00 00 01 .....Ue. ........
    00000022  00 00 00 00                                      ....
00000014  49 48 41 56 45 4f 50 54  00 00 00 07 00 00 00 0c IHAVEOPT ........
00000024  00 00 00 04 74 65 73 74  00 01 00 03             ....test ....
    00000026  00 03 e8 89 04 55 65 a9  00 00 00 07 80 00 00 01 .....Ue. ........
    00000036  00 00 00 00                                      ....
00000030  49 48 41 56 45 4f 50 54  00 00 00 03 00 00 00 00 IHAVEOPT ........
    0000003A  00 03 e8 89 04 55 65 a9  00 00 00 03 00 00 00 02 .....Ue. ........
    0000004A  00 00 00 04                                      ....
    0000004E  74 65 73 74 00 03 e8 89  04 55 65 a9 00 00 00 03 test.... .Ue.....
    0000005E  00 00 00 01 00 00 00 00                          ........ 
00000040  49 48 41 56 45 4f 50 54  00 00 00 02 00 00 00 00 IHAVEOPT ........

The commands are

NBD_OPT_STRUCTURED_REPLY →
← NBD_REP_ERR_UNSUP
NBD_OPT_GO →
← NBD_REP_ERR_UNSUP
NBD_OPT_LIST →
← response with invalid `length=4` field - response data is longer than 4 bytes
NBD_OPT_ABORT → 

The NBD protocol docs says:

NBD_OPT_ABORT (2)

The client desires to abort the negotiation and terminate the session. The server MUST reply with NBD_REP_ACK.

However, this issue probably won't cause problems, because

Previous versions of this document were unclear on whether the server should send a reply to NBD_OPT_ABORT. Therefore the client SHOULD gracefully handle the server closing the connection after receiving an NBD_OPT_ABORT without it sending a reply. Similarly the server SHOULD gracefully handle the client sending an NBD_OPT_ABORT and closing the connection without waiting for a reply.

According to the NBD protocol we must do a soft disconnect in this case.

• Check where this is used currently. If it's called from somewhere, run the relevant tests with this fix.
• When this is fixed, some of the test cases marked with TODO may need to be updated to remove the workaround that was added due to this bug.

As @mseri noted, the code needs more of these. So we should add them after we've moved to version 3 and finalized the interface. Somebody less familiar with the code could help us decide what to comment.

By default the Makefile runs ocaml setup.ml -configure --enable-tests, which fails if the test dependencies are not installed.

Perhaps this is a good time to transition to topkg?

See [ocaml/opam-repository#7913]

The NBD protocol says about NBD_CMD_WRITE:

A write request. Length and offset define the location and amount of
data to be written. The client MUST follow the request header with
length number of bytes to be written to the device.
The server MUST write the data to disk, and then send the reply
message. The server MAY send the reply message before the data has
reached permanent storage.
If an error occurs, the server MUST set the appropriate error code
in the error field.

And about disconnecting during the transmission phase:

Either side MAY initiate a hard disconnect if it detects
a violation by the other party of a mandatory condition
within this document.

and:

The client and the server MUST NOT initiate any form
of disconnect other than in one of the above circumstances.

And about NBD_FLAG_READ_ONLY:

The server MAY set this flag to indicate
to the client that the export is read-only (exports might be read-only
in a manner undetectable to the server, for instance because of
permissions). If this flag is set, the server MUST error subsequent
write operations to the export."

Thus it seems that the server should not disconnect in case of
NBD_CMD_WRITE errors, but should continue processing the client's
requests.

While testing v2.1.3 we discovered that it does not recognize the NBD_FLAG_FIXED_NEWSTYLE flag correctly. I think the newest 3.0.0 version is also affected, because its code has the same issue:

I think the issue is that the NegotiateResponse.unmarshal function in lib/protocol.ml decodes the client flags as a little-endian integer (Cstruct.LE. ...), but according to the NBD protocol all these flags and ints are in the network byte order, which is big-endian. Changing Cstruct.LE to Cstruct.BE fixes the issue.

The NegotiateResponse.marshal function has the same issue, which also incorrectly uses Cstruct.LE, and this is used by the client for sending the client flags to newstyle servers (for connecting to them).

On Ubuntu 15.10, I run the nbd server, then try and connect with the ubuntu "nbd-client". I can expose the remote block device on /dev/nbd0, but when I try to use the device (eg by formatting to ext4) the mkfs command hangs.

Should I expect this software to work? To work with the particular client ubuntu uses? Some other client?

Any help appreciated!

io-page-unix isn't available in latest io-page
see mirage/io-page@1136fa7

Currently we do these synchronously: we read from the block device, wait for the data to arrive, and then we write it, and wait for the data to be sent.
But this is inefficient, we could be sending the data and reading the next chunk of data at the same time.
We could use a fixed-size circular buffer for this producer-consumer scenario:
https://en.wikipedia.org/wiki/Circular_buffer