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>

Use a C99-style variadic macro rather than a variadic function

Since that function effectively only really calls one other function,
that should be somewhat more efficient.

nbd-server: replace msg2(), msg3() and msg4() with variadic msg()

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

nbd-server: remove unused function serve_err()

Error reporting and handling should be streamlined. Currently there are
quite many different ways errors are being logged, reported and handled.

This shall be the first baby step towards more uniform error management.

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

nbd-server: fix server socket initialization

Change the value for undefined descriptor from 0 to -1 because any value
greater or equal to 0 can be a valid file descriptor.

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

Remove return statements after calls to err()

err() does not return, but exits with error code

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

Fix typo in include guard test

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

Fix fallocate call

If we want to use FALLOC_FL_PUNCH_HOLE, the documentation tells us we
also need to use FALLOC_FL_KEEP_SIZE.

Discovered-By: Nicolas Trangez

Fix off-by-one in exptrim

Otherwise we may miss (part of) a TRIM command if it happens to fall on
a file boundary (or on the start of a single-file export)

Discovered-By: Nicolas Trangez

Add NBD_CMD_TRIM to getcommandname

Discovered-By: Nicolas Trangez

Prepare for release

nbd-server: treat accept errors as non-fatal

On Sun, Jun 24, 2012 at 01:31:28AM +0200, Wouter Verhelst wrote:
> On Fri, Jun 22, 2012 at 01:40:48AM +0400, Dmitry V. Levin wrote:
> > In most cases, an error returned by accept system call is a network
> > error, which should not terminate the whole nbd-server.
>
> Yes, that's a leftover from the days when nbd-server ran in
> one-process-per-export mode (i.e., the 2.7 days). Oops.

Then there is at least one more issue of this kind:

>From a2e158154fd743fb039e0823736774804a9645ac Mon Sep 17 00:00:00 2001
From: "Dmitry V. Levin" <ldv@altlinux.org>
Date: Sun, 24 Jun 2012 22:40:54 +0000
Subject: [PATCH] nbd-server: treat getpeername errors as non-fatal

In most cases, an error returned by getpeername system call is a network
error, which should not terminate the whole nbd-server.

Fix test syntax, and add 'set -e' so that if we do this again in the future, we fail the test suite.

Remove redundant __linux__ #ifdef lines

nbd-client is Linux-only, so we don't actually need to check for that.

nbd-server: fix signal handling

Reset SIGCHLD and SIGTERM handlers to SIG_DFL in child processes because
it is not their business to handle these signals.  Block SIGCHLD and
SIGTERM for the short time of fork, changing signal handlers and
changing "children" hash table to avoid race conditions with nasty
consequences.  Fix SIGTERM handler to always unlink pidfile.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

nbd-server: block SIGCHLD when handling SIGTERM, and visa versa

SIGCHLD handler must be blocked during execution of SIGTERM handler,
and visa versa, because the former changes "children" hash table
iterated by the latter.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

nbd-server: fix several memory leaks in connection handling code

This change fixes a CLIENT leak on normal path in the parent process,
several CLIENT leaks on error path in the parent process, a pid_t leak
on error path in the parent process, and a pid_t leak in the child
process.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

nbd-server: do not call fcntl F_SETFL unnecessarily

Call fcntl F_SETFL only when new file status flags differ from the old one.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

nbd-server: do not lose connections

When select reports that more than one connection is ready to be
accepted, handle them all instead of losing all of them but one.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

nbd-server: move connection handling code to a separate function

The serveloop function has grown too much, and the indentation of
connection handling code unnecessarily complicates further changes.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

nbd-server: treat accept errors as non-fatal

In most cases, an error returned by accept system call is a network
error, which should not terminate the whole nbd-server.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

nbd-server: fix accepted connections descriptor initialization and use

Change the value for undefined descriptor from 0 to -1 because
any value greater or equal to 0 can be a valid file descriptor.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

nbd-server: avoid modernsock descriptor leaking to child processes

Close modernsock descriptor in child processes.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

nbd-server: fix modernsock descriptor initialization and use

Change the value for undefined "modern handler" descriptor from 0 to -1
because any value greater or equal to 0 can be a valid file descriptor.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

nbd-server: zeroise hash table pointer after destroying the table

This is required to avoid accessing freed data later in
sigterm_handler.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

nbd-client: try harder to reconnect in persist mode

When nbd-client in persist mode is trying to reconnect because of
nbd-server restart, it is highly probable that nbd-server will not be
ready to serve a reconnection request in time causing nbd-client to exit
with "connection refused" diagnostics.  This change makes nbd-client in
persist mode retry its reconnection attempts until succeeded.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

nbd-client: adjust the oom-killer score in swap mode

In swap mode, it is highly desirable to take all available measures that
could help nbd-client survival in low memory conditions.  One of such
measures is to disable oom killing of nbd-client by adjusting the
oom-killer score.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>

nbd-client: do not leave child processes in the zombie state

Change SIGCHLD handler to SIG_DFL with SA_NOCLDWAIT flag set, to avoid
transforming child processes into zombies when they terminate.  Since
the main process is inevitably blocked in NBD_DO_IT ioctl at the time
when child processes terminate, this is the simplest way to get rid of
zombie processes.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
Signed-off-by: Wouter Verhelst <w@uter.be>

Don't kill processes unless we're sure there's actually a process to kill

Add some more logging

refer to the correct manpage

Mark test as skipped, rather than as successful

Fix a minor memory leak

The 'next' label does some cleanup, so jump there rather than to the
start of the loop.

Add test for "list exports"

occasionally, since this uses the regular nbd-client, it also tests
whether it will at least do something useful, and not crash and burn at
startup. We can't test whether it will connect properly, since we're not
sure we have the environment set up for that completely, but this is a
good idea regardless.

Updates

Prepare for 3.1.1

Initialize stack variables opts and cflags.

Without initializing these variables, nbd-client will some of the time
list exports instead of connecting to them, depending on the wing flaps
of a butterfly and cosmic alien rays. The fix is to set the flags to 0
so that nbd-client operates reliably.

Group local headers together beyond the system headers and library headers.

compat: Define AI_NUMERICSERV as `0' if necessary

AI_NUMERICSERV as a value for the `ai_flags' member
of `struct addrinfo' of header <netdb.h> has only
been available since:

  POSIX 1003.1-2008, Issue 7
  glibc 2.3.4
  Mac OS X 10.6
  etc.

Fortunately, its main purpose seems to be only
to optimize calls of `getaddrinfo', and because it
is meant to be a bit flag, it can therefore be
[relatively] safely ignored by defining it to have
the value zero.

Signed-off-by: Michael Witten <mfwitten@gmail.com>

Initialize needed_flags

otherwise we may end up sending garbage to the negotiation.

Remove outdated comment

Documentation update for list exports

Prepare for 3.1

Don't overdo it on the output

"make check" would produce massive amounts of data, that we don't always
need; and if we do, there's script(1).

Meanwhile, this makes sure we can still scroll up and see what happened
with previous tests.

Fix alignment issues and build on SPARC

nbd-tester-client, in the "integrity" test, was making some assumptions
that were incorrect on architectures such as SPARC which care about
alignment. This patch fixes those, by making sure we always access
64-bit values through a uint64_t, rather than through a char.

Don't forget to convert to network byte order

Make output somewhat more pretty

Implement "list exports" message

Previously, there was no way for the client to get a list of all
available exports from an nbd-server. This implements that.

Additionally, it implements a way for the server to understand more
options than just the one "NBD_OPT_EXPORT_NAME" we did before, and
proper handling of "sorry, I don't know what you're talking about".

Fix reference to INSTALL file -- oops, that was still open

Fix for device name detection

Our nbd devices end up in /dev/block/nbd*. check_conn() tries to
derive the path to the pid file under /sys/block/<nbd device>/pid by
stripping a leading /dev/ path prefix. In our case it ends up with
/sys/block/block/<nbd device>/pid and is unable to locate the pid file.
The attached patch nbd-client-devname.patch uses

        if( (p=strrchr(devname, '/')) ) {
                devname=p+1;
        }

to derive the name of the device.

Fix for error handling

When a client disconnects from the nbd device (triggered via nbd-client
-d <nbd device>) the ioctl(nbd, NBD_DO_IT) returns with an error.
nbd-client checks errno to figure out whether it was disconnected and
whether it should terminate or not. Unfortunatly it invokes fprintf()
before checking errno and fprintf overwrites the current value of errno.
The attached patch saves the current value of errno and checks the saved
value.

Use the formatted string, not the unformatted one.

Use less syscalls

If we've just done ftruncate, then either we were successful and know
the new file size (and then we don't need an lseek), or we weren't and
then we need to error out anyway.

Doing things this way also avoids another warning in the code.

Transition do_oldstyle variable to a global flags mask

... in preparation for more global options.

Use assert rather than g_assert

Support filesystems that don't return filetypes in readdir()

Do the check_conn in the child, not the parent

Release 3.0

Also, pull a Linus -- the version number does not seem to make much
sense anymore, so change to 3.x version numbers rather than trying to
stick to the 2.x.y one. Not because it's a major update, but because I
want to start making sense again.

Merge pull request #5 from tabraham/master

properly check for HAVE_FALLOC_PH define in both occurrences

properly check for HAVE_FALLOC_PH in both occurrences

Make assertions about state in negotiation function

Improve indentation to avoid this kind of thing from happening again.

nbd-server: use modern alternative to legacy index() function

index() was marked as obsolete by susv3, and isn't available on uClibc
in some configurations, so use the equivalent strchr() instead:

http://pubs.opengroup.org/onlinepubs/009695399/functions/index.html

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>

lfs.h: fix !LFS builds

Commit 448637fba4 (Fix 32/64 bit confusion) broke !LFS builds as it
added the PARAM_OFFT defines inside the wrong conditional.

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>

Check for defines the right way

I keep forgetting that autoconf says '#define FOO 0' rather than '#undef
FOO'...

Release 2.9.25

Use proper values

.required is a gboolean, so specify its value as FALSE rather than 0.
The result is the same, but this reads better...

Fix this check

Due to a mistake, it was placed in the "if (virtstyle)" case, rather
than always. Whether a user is specifying a virtualization style has
nothing to do with whether they should specify a port number...

Update manpage

There were some confusing examples at the bottom of the manpage that
were still showing the old ways of doing things. Update them so they no
longer show 'port=' parameters without also showing 'oldstyle=true'.

Via https://forums.gentoo.org/viewtopic-t-896132.html?sid=51ce5d51b9c62fc5d9eecfdaa988ceef

Bump required version of libglib

Use msg3 instead of msg2 (oops, my mistake)

Fix msg2 calls

Save and restore errno before calling functions

not doing this caused problems on Debian GNU/kFreeBSD

Implement TRIM as fallocate(FALLOC_FL_PUNCH_HOLE) call

Compile-tested, but not yet functionally tested.

Fix check for FALLOCATE_FL_PUNCH_HOLE

- We were using AC_COMPILE_IFELSE when there was a perfectly good
  AC_CHECK_DECL specifically for this purpose.
- The check for the result of the header file check used the wrong name
  for the cache variable, so always returned false, so we never checked,
  in fact. D'oh.

It now properly returns true when needed

Remove unused variable

Add test for config file snippet stuff, and make the server pass it.

Document trim option in manpage, too.

add support for NBD_CMD_TRIM, update docs

Update the protocol documentation, and implement it as a dummy
command in the server.

Fix error handling