Make things work again

Tickle the git repository

Did something wrong, hopefully it will work now.

Test build failure mail thing

buildbot has a "send mail upon build failure" option. This should test that.

We'll revert the break once everything seems to work ;-)

Make the implementation of size_autodetect follow the documentation

The documentation claimed that size_autodetect could return UINT64_MAX
if size autodetection would fail. In fact, however, it would call err(),
which is for fatal errors.

Additionally, the final test (to use lseek SEEK_END) would never be
called, since it was preceded by an if structure which would call return
in one of its branches and err() in the other.

Remove off_t from what will eventually be a public API

Since the size of off_t may vary depending on whether the LFS defines
have been set, it is not a good idea to expose that, as the recently
fixed bug has shown. As long as we use nbdsrv.h internally only this
doesn't matter, but since the long-term goal is to make libnbdsrv a
public library, we'd better avoid such an issue.

We'll still use off_t internally, but that won't be exposed anymore.

Remove duplicate ISSERVER definition

Reorder LFS includes so it actually works

Add missing lfs.h include

lfs.h has an influence on sizeof(off_t), which has an influence on the
offsets of the SERVER data type, which makes things fail on 32 bit...
whoops.

Fix cleanup in simple_test

If nbd-server dies during the test for some reason or other, then the
kill call in the cleanup subroutine would fail, causing (due to 'set
-e') the script to abort immediately. As a result, the "rm -rf" of the
tempdir wouldn't happen, which would leave cruft in /tmp.

Fix.

Fully initialize hints

POSIX 1003.1 says:

 In this hints structure every member other than ai_flags, ai_family,
 ai_socktype, and ai_protocol shall be set to zero or a null pointer.

which Linux doesn't enforce, but FreeBSD does.

Fix missing include

Linux automatically includes <netinet/in.h> as a dependency of one of
the other include files, but this is not required by POSIX, and FreeBSD
doesn't do so.

Since we directly use some of the structs from that file,
add the include

Prepare for 3.6

Switch to xz (and away from bzip2)

Might change back, depending on discussion on mailinglist.

Fix build

If we introduce a new file, we need to make sure to add it to the
Makefile, too, otherwise it won't be distributed...

Add check for size_autodetect

Move size_autodetect to library

Add check for getmaskbyte(), too

Don't forget to distribute this new "macro.h", too

Add a check for append_serve, too

Separate out a useful macro

Update comment to reflect today's realities

Move "append_serve" to library, too.

That completes what can be moved without rewriting anything.

Whitespace fix

Fix Makefile.am's so that "make distcheck" works again

Add .gitignore with compiled files in this directory.

Make it compile (and pass)

Add test for dup_serve

Move dup_serve() to library

Enable maintainer-mode by default

Do a htonl conversion of opt.

Closes github issue #6, but breaks backwards compatibility on
little-endian systems.

I've decided not to care anymore about that. Cross-endian same-version
compatibility matters more than backwards compatibility.

So there.

Add a "code" test suite

This test suite will test functions in libnbdsrv.la. For now, only test
that the authorized_client function does what it's supposed to do.

Make a start with a library

The library does not contain much useful stuff yet, so make it a
noinst_LTLIBRARY for now, rather than a lib_LTLIBRARY.

As we move code to the library, we'll modify it so that it becomes
selfsufficient, and clean it up as we go.

For now, only move the freshly rewritten authorized_client() function
there.

This adds libtool to the list of stuff needed when compiling from git,
but that's not much of an issue.

Separate the test suite out into a separate directory

... in preparation of also adding a "code" test suite which tests
functions in the code, not runtime behaviour of nbd-server

Bring documentation in sync with code

Improve acl file parsing robustness

We now support comments, empty lines, and leading whitespace (which all get
skipped)

Lines with junk will _probably_ get skipped too, depending on how bad
the junk really is.

Unbreak virtstyle parameter parsing

There have been issues with the "cidrhash" option of the "virtstyle"
parameter since a long time, in that the way things were being parsed
wasn't very endian-independent.

Fix it the same way we just fixed the authorized_client function.

Fix authorized_client implementation a bit more

We previously added a fix for CVE-2013-6410, but that one accidentally
broke checks for non-netmasked entries entirely, rather than limiting it
to what it was actually supposed to allow. '\n' is also a character...

This fixes it properly, by not relying on string functions anymore, but
instead parsing all IP addresses to their binary representation, and
comparing that.

As an added bonus, we now also support IPv6 addresses in the ACL file.

Merge pull request #15 from alexsn/master

Fix potential socket leak in opennet()

gznbd: fix warning with newer zlib

When you build against newer zlib, you get a lot of warnings like so:

gznbd.c: In function 'main':
gznbd.c:87:5: warning: assignment from incompatible pointer type [enabled by default]
gznbd.c:109:5: warning: passing argument 1 of 'gzread' from incompatible pointer type [enabled by default]
In file included from gznbd.c:37:0:
/usr/include/zlib.h:1313:21: note: expected 'gzFile' but argument is of type 'struct gzFile_s **'
gznbd.c:118:9: warning: passing argument 1 of 'gzerror' from incompatible pointer type [enabled by default]

This is because the zlib API uses just gzFile everywhere, not a pointer
to a gzFile.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Wouter Verhelst <w@uter.be>

gznbd: use PRId64 for printing 64bit types

Gcc complains about printing these 64bit types:

gznbd.c:199:1: warning: format '%Ld' expects argument of type 'long long int',
but argument 5 has type 'u64' [-Wformat]

Use the standard defines from inttypes.h to avoid this issue.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Wouter Verhelst <w@uter.be>

Add the actual text of the GPLv2, as the license requires us to

Update nbd-client.c

Fix potential socket leak in opennet()

Prepare for 3.5

Use strcmp() rather than strncmp()

This results in some false positives. If the authfile contains:

192.168.0.12

and the client is

192.168.0.1

then access will be granted, because the strcmp was limiting to
"192.168.0.1" for no particularly good reason.

We should also canonicalize our names and work on that, rather than
doing a simple strcmp(), but that's for later.

Fix documentation to reflect reality

3.4 was released...

re-enable integrityhuge test

We disabled it at the time because it would deadlock in some cases
(though not on my machine).

Since then, some deadlock fixes have accumulated. I don't know whether
the culprit has been found and remedied, but it doesn't hurt to try. If
it turns out to still be a problem, we can always disable them again
afterwards.

While here, remove generated files ("missing") from git.

Remove the same ioctl() call in another place

Not sure why we do this twice anymore, but hey.

Signed-off-by: Wouter Verhelst <w@uter.be>

Remove obsolete NBD_CLEAR_QUE ioctl.

Signed-off-by: Wouter Verhelst <w@uter.be>

nbd-server: const'd function parameter variable

Making the parameter variable const revealed a smelly assignment
statement which was deemed unnecessary and hence removed.

Signed-off-by: Tuomas Räsänen <tuomasjjrasanen@tjjr.fi>
Signed-off-by: Wouter Verhelst <w@uter.be>

nbd-server: remove superfluous 2nd parameter from handle_oldstyle_connection()

A pointer to the server struct is passed anyways, the socket can be
accessed via that pointer.

Signed-off-by: Tuomas Räsänen <tuomasjjrasanen@tjjr.fi>
Signed-off-by: Wouter Verhelst <w@uter.be>

nbd-server: refactor out common code

Signed-off-by: Tuomas Räsänen <tuomasjjrasanen@tjjr.fi>
Signed-off-by: Wouter Verhelst <w@uter.be>

nbd-server: rename handle_connection() -> handle_oldstyle_connection()

The new name describes better the actual usage.

Remove also the fourth parameter, CLIENT*, which was always NULL.

Signed-off-by: Tuomas Räsänen <tuomasjjrasanen@tjjr.fi>

nbd-server: handle modern-style negotiation in a child process

Previously, the modern style negotiation was carried out in the root
server (listener) process before forking the actual client handler. This
made it possible for a malfunctioning or evil client to terminate the
root process simply by querying a non-existent export or aborting in the
middle of the negotation process (caused SIGPIPE in the server).

This commit moves the negotiation process to the child to keep the root
process up and running no matter what happens during the negotiation.

See http://sourceforge.net/mailarchive/message.php?msg_id=30410146

Signed-off-by: Tuomas Räsänen <tuomasjjrasanen@tjjr.fi>

Clarify that we're using tests which expect the serial test protocol

Automake changed its defaults from using the serial protocol to using
the parallel protocol sometime between 1.11 and 1.13.

Our test suite makes a few assumptions which are valid for the serial
protocol, but not for the parallel protocol, which makes the test suite
fail with 1.13. As such, explicitly state that we want the serial
protocol so that things don't break with recent versions of automake.

A proper fix would be to make the test suite work with the parallel
protocol, but that's a lot more work than we have time for right now.

nbd-server: narrow the scope of a function-local variable

It's better to keep variables strictly inside their usage scope than in
more wider scope to avoid unintentional variable shadowing and leaking.

Signed-off-by: Tuomas Räsänen <tuomasjjrasanen@tjjr.fi>

nbd-server: check the right socket variable for an invalid value

The previous check referred to a variable which was used only for
initializing the fd set and wasn't meaningful in the context of the
invalidity check.

Signed-off-by: Tuomas Räsänen <tuomasjjrasanen@tjjr.fi>

This was fixed a while back for 3.3, but I forgot to remove the TODO item.

Don't dump core *everywhere*

It may make sense to dump core if I'm doing some debugging, but that
doesn't mean we should have anyone anywhere do that.

Fix my own stupidity

Michal's patch didn't apply cleanly, so I "applied" it manually, and
didn't think to compile-test.

Obviously next time I should just reject patches that don't apply. Oh
well.

Ignore SIGCHLD rather than blocking it

Since commit 3927df64, we're blocking most signals. Blocking a signal
implies it remains pending until we unblock the signal, at which point
it will be handled.

While that is the right choice for some signals, we're really not
interested in SIGCHLD, so ignore that instead. This makes our zombies go
away.

Buffer overflow and unterminated string in nbd-client.c

Fix port spread

We were attempting to make sure no test used the same port twice, but
still reused the same number once. This fixes that.

Distribute these files, too

Prepare for 3.3

Remove long-outdated conditional code

Improve signal handling

Setting SIGCHLD to SIG_DFL doesn't cut it; we need to ctually ignore
them with sigprocmask. In addition, we need to deal with more than
SIGCHLD.

Initial patch by Paul Clements.

Fix ordering

FreeBSD wants non-option arguments to appear after option arguments

Don't try to check whether invalid sockets are readable

this caused SIGBUS on FreeBSD

Don't attempt to free a NULL pointer.

Open as many modern sockets as needed

Some systems need us to have a socket per address family (e.g.,
FreeBSD); others need us to have just one (e.g., Linux). Handle both
cases.

Handle portability better.

Yesterday's attempt was somewhat shortsighted. This one should actually
work.

We avoid the ugliness of having to do the same thing twice somewhat by
putting it in a separate function -- this is still ugly to some extent,
but at least it doesn't copy code around more than just for the function
name.

Revert "Be more portable"

This reverts commit 77ee0f392eaf292f2c0d350fa5c640253d2132d7.

It doesn't work on FreeBSD, and it doesn't work on Linux. Oops.

Add todo item so we don't forget to fix this later

More portability fixes

It still doesn't work on FreeBSD, but I just ran out of time

Clean up in all cases

Since we use 'set -e', there are plenty of cases where the cleanup code isn't called.
Use 'trap' to ensure that it does.

Be more portable

Using the leading - is a GNU extension (but not documented as such), which
 doesn't work on FreeBSD. Use the more portable way to do the same thing.

Use the correct error handling routine

Use $TMPDIR rather than hardcoding for /tmp

Add a template to mktemp invocation

Not all mktemp implementations (e.g., the FreeBSD one) have an implicit
template. Add one explicitly so things will work properly again.

Ensure we don't bring the '-l fucks up nbd-server' bug again, by adding it to the test suite

nbd-server: prefer assertions over if-then-exits

Assertions imply programming invariants which must be fulfilled,
otherwise there is a programming error.

Regarding this commit, it's a programming error to pass NULL to
append_serve(). And now it's explicitly stated with the assert(3)
statement.

If-then-exit -constructs could be interpreted as a valid and
semantically meaningful code: "it's a feature of append_serve() to
exit the process if NULL is passed", which does not make any sense. On
the other hand, assert(s != NULL) makes it very clear that NULL must
not be passed, or the contract will be terminated along with the
process.

This commit is part of the desensitization treatment for author's
assertion allergy.

Signed-off-by: Tuomas Räsänen <tuomasjjrasanen@tjjr.fi>
Signed-off-by: Wouter Verhelst <w@uter.be>

nbd-server: read conf on SIGHUP and add new exports

This patch makes the root server process re-read its configuration files
and add new exports dynamically when SIGHUP is sent. The rehash feature
is non-destructive: it does not modify any existing exports, it only
lets the root server process to add new exports. However, it might be a
good idea to implement destructive counterparts (export deletion and
modification) too at some point.

Signed-off-by: Tuomas Räsänen <tuomasjjrasanen@tjjr.fi>
[change usage of __attribute__((unused)) into G_GNUC_UNUSED]
Signed-off-by: Wouter Verhelst <w@uter.be>

Merge these two tests

Testing for "larger than the max value" is impossible and silly, but is
what the next test actually tests. Remove the erroneous one.

Remove erroneous cast

Don't remember why it's there, it's probably a thinko.

Pointed out by Goswin von Brederlow <goswin-v-b@web.de>

BUG: oversized 64b offset wrap not detected when offset + len > 64bit and thus wraps

if:
offset = 64bit - 2KB
len = 4KB
then the server will allow the read/write because the check
if (((ssize_t)((off_t)request.from + len) > client->exportsize))
will never trigger as client->exportsize will be compared with... offset
2KB!

BUG: debug output uses signed integer for length field

Replaced %d by %u.

BUG: read-command with data size 0 does not return a response header

NBD-server v3.2 has a problem: if a client does a read-request of 0
bytes in length, then the nbd-server does not return a response header.
This is not in line with a write request which always returns a response
header.
This patch solves that behaviour.

More manpage updates

Remove cryptic error

We already made it slightly less cryptic when you tried to connect to a
newstyle server without a name for the export. Do the same for the
reverse, too.

Clarify error handling.

While working with nbd-server, I tried to allow the clients to
fetch a list of exports. After including the following in the
generic section of /etc/nbd-server/config:
[generic]
# other lines ...
allowlist
nbd-server refused to start, complaining there were no configured
exports, even though there were:
nass0:root ~ 17 # nbd-server -d
** Message: No configured exports; quitting.

Obviously, the line should read 'allowlist = 1', but It would be
helpful if nbd-server detected the error, and complained about
that instead of (apparently) ignoring the rest of the config file,
and issuing the confusing complaint about no configured exports.

I also tried 'allowlist = yes'. In that case it gives a
descriptive message, although it still confusingly and incorrectly
complains that there are no exports:
nass0:root ~ 14 # nbd-server -d

** (process:3482): WARNING **: Could not parse config file: Could not parse allowlist in group
generic: Key file contains key 'allowlist' which has a value that cannot be interpreted.
** Message: No configured exports; quitting.

The cause seems to be that g_key_file_load_from_file fails not only
if the file cannot be found or read, but also if it contains syntax
errors.

Original patch by Rogier <rogier777@gmail.com>, but reworked enough by
$SELF that it isn't the same thing anymore.

Update manpage to reflect reality

Originally, it wasn't allowed to specify both a port and a name. It is
now, so update the manpage.

nbd-server reports 'negotiation failed' after 'nbd-client -l', and fails on next connection

When a client requests an export list from the server,
the latter issues the message 'Error: negotiation failed'.
This is rather confusing, as the only 'problem' is (AFAICT)
that the negotiation phase did everything.

On a subsequent attempt to connect a block device, the server
fails altogether:
Client output:
--------------------
nasc0:root ~ 216 # /tmp/nbd-client.master -l nass0
Negotiation: ..
nasc0-nbd0
nasc0:root ~ 217 # /tmp/nbd-client.master nass0 -N nasc0-nbd0 /dev/nbd0
Negotiation: ..size = 856941734MBError: Exported device is too big for me. Get 64-bit machine :-(

Exiting.
--------------------
Server output:
--------------------
nass0:root ~ 135 # /tmp/nbd-server.master -d -C /etc/nbd-server/config
Error: negotiation failed
Exiting.
Error: Read failed: Connection reset by peer
Exiting.
--------------------
From a tcpdump, it can be observed that the syslog messages end up
on the client socket, which makes the client choke, naturally.

I tested this for the latest git version, and the same problem
occurs. I have traced the problem to the fact that negotiate(),
in two locations, closes the socket, and returns NULL , after which
serveloop also closes it, actually closing the syslog socket instead.
The next accept() returns that same filedescriptor, which syslog()
still thinks it owns.

Note: I added the message 'Session terminated by client', because
the 'ABORT' might also be intended to be used as an irregular
end-of-session (in the future?), besides indicating a regular
end-of-session after a LIST.

--------------------------------------------

Add missing closing bracket (whoops).

Spotted by: Michal Belczyk <belczyk@bsd.krakow.pl>

Document that people should really use newstyle when possible.

Be a bit more helpful when someone tries something silly.

Bring documentation in sync with code.

Spotted by: Juan J. Martínez

Merge pull request #12 from reidrac/master

Detect server disconnection after NBD_OPT_EXPORTNAME

actually check that read may return zero on EOF

Detect server disconnection after NBD_OPT_EXPORTNAME

If the export is unknown to the server, it may disconnect (new-style handshake
un-fixed). In that case the read call to get export size may return 0 (non error, EOF).

This change fixes following error:

    nbd-client -N unexistent 127.0.0.1 /dev/nbd0
    Negotiation: ..size = 2314498962MBError: Exported device is too big for me. Get 64-bit machine :-(

    Exiting.