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.

nbd-server: handle port number string conversion errors

g_strdup_printf() tries to allocate a string large enough to hold the
string representation of the passed value and coverts the value to
string. It returns NULL if memory allocation fails, or some other error
occurs. Previously, NULL was interpreted as "modern style socket not
needed for this export" by returning zero to the caller, which was
utterly wrong.

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

Add doc/* to distributed files.

Add TODO file.

This contains just what I could remember off the top of my head, but I'm
fairly sure there's lots of stuff missing. I'll add to the list as I
remember them.

nbd-server: use one error domain for all NBD server errors

The purpose of the commit is to move all error identifiers to one
namespace common for all errors originating from NBD server. Using
multiple internal error domains would add only bureaucracy without any
real gain. Error domains should be used to separate errors between
modules, not within modules. For example, NBD client would define its
own error domain and codes.

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

nbd-server: fix setup_serve() to report errors

This commit changes setup_serve() to report its errors via GError object
to its caller. It also add resource deallocation code to return points
to free all local resources correctly. Note that, the previous version
of the setup_serve() did not free its resources properly on error, but
just called functions which terminated the process. This was techincally
a leak, but without any practical significance due to immediate process
termination. However, now that error handling (process termination) is
separeted from error reporting, we must deallocate/free all locally
allocated resources correctly on errors.

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

nbd-server: fix open_modern() to report errors

This commit is part of the refactoring work on internal error
management. The goal of the work is to separate error reporting from
error handling to make it possible to call functions from different code
paths with varying needs for error handling.

Another goal is to unify the error management throughout the code base.

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

nbd-server: remove commented code

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

nbd-server: remove unused function

Something like this might be needed in the future, once dynamic
reconfiguration has landed. But it's better to rewrite and test it then
and not keep unreferenced code hanging around cluttering the code base.

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

Remove unnecessary free() calls

Technically this is a leak, but it does no harm to leave them there:
- It may aid in debugging
- future features (try pronouncing that! ;-) may use these values for
  other purposes
- additionally, while the pointers are freed, they're not set to NULL,
  which makes them unsafe dirty pointers. That's actually worse than not
  being freed, in my opinion.

nbd-server: remove global variables: modern_listen and modernport

Address and port of the modern socket are read from the main
configuration file and used only when opening the socket. There is no
need to keep them hanging in the global scope.

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

nbd-server: remove global variables: runuser and rungroup

User and group names are needed only at the very beginning of the
process, there is no need to keep them hanging in the global scope.

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

nbd-server: fix parse_cfile() to not modify any global variables

This change allows parse_cfile() to be called without any side-effects
and passes the decision of what to do with parsed results to the
caller.

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

nbd-server: remove return statement after call to serveloop()

serveloop() never returns, now it is also marked as such.

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

nbd-server: fix append_serve() to accept a const server pointer

Now that dup_server() accepts const pointer, append_serve() can also
accept const pointer. There should not be any need to modify the server
when appending it to the array.

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

nbd-server: fix dup_serve() to accept a const server pointer

dup_serve() does not modify the source server (as it should be), so the
pointer should be declared const.

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

nbd-server: fix dosockopts() to report errors

The purpose of this change is to separate error reporting from error
handling and hence make dosockopts() usable also in other calling
contexts. dosockopts() reports errors in setting socket options and then
it's up to the caller to decide how to react to those errors.

This is a step towards more uniform and robust error management. Similar
changes needs to be done to other functions as well.

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

nbd-server: remove unused CFILE_PROGERR error code

Programmer errors should not be mixed with runtime errors. If there's a
prorammer error, then there's nothing we can do about it during runtime,
and the program should fail as early and loud as possible.

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