Stop using nonportable flags of readlink

"readlink -f" is a GNU extension. On OSX (where we try to run the test
suite, too, courtesy of travis CI) it does not exist.

Not sure why we ever added it in the first place (symlinks work, you
know) so just drop it entirely; there is no need to canonicalize things
here.

Allow for things to already be installed

Our "brew install gnutls" thing is currently failing with "error, gnutls
is already installed, use upgrade to go to the latest version".

While an antifeature in brew, it is easily worked around.

Don't upgrade things we don't actually need

"brew upgrade" tries to upgrade a whole bunch of stuff. We don't need
it, and it causes failures, so stop trying.

Stop working around ubuntu precise outdatedness

.travis.yml still contained some stuff that was there because the
precise images that travis used to use were too outdated for things.
However, travis has since upgraded to trusty, and so the issues that we
were dealing with now longer are problematic.

Drop it.

Add a few dependencies that we need...

libgnutls-dev is preinstalled on the trusty images of travis, but points
to GnuTLS 2.12 rather than something more modern. Presumably this is why
things break so badly.

Explicitly install libgnutls28-dev to fix. While at it, also install
libnl-genl-3-dev to make sure we compile-test the netlink stuff.

Handle execvp() failing in the tester client

(cherry picked from commit 0b8a27bc6bbde02674e6ad3269d0fa7c5ca2f569)

Merge pull request #76 from fstirlitz/inetd-mode-fix

inetd mode fix; resolves #75

nbd-client: fix no-optgo compilation

The changes to get_from_config() and its caller were incomplete,
causing the compiler to complain about type mismatch and choke
on a missing parameter.

Fixes: dc997328
Signed-off-by: Eric Blake <eblake@redhat.com>

doc: Clarify maximum size limits

The existing text on block sizes had a redundant statement about
hard disconnects on large requests, but did not specify a size
where that comes into play.  In practice, several NBD
implementations have settled on 32M (qemu, kernel module) or 64M
(nbdkit) as the limit for maximum request size (at the client)
or hard disconnect size (at the server); thus we should document
32M as the portable limit that clients can expect and servers
should support as a way to increase interoperability.

There is also some confusion on whether WRITE_ZEROES must abide
by the same limits as WRITE.  In practice, servers permit larger
limits for TRIM and WRITE_ZEROES than for WRITE because there is
no payload involved; but a server may still want to enforce a
maximum size other than 4G because of other constraints (for
example, if a WRITE_ZEROES request is implemented by malloc'ing
a buffer and performing a naive write, clamping the maximum
malloc size or limiting the amount of time spent on looping
to perform the writes may still be desirable for the server).
But in practice, existing servers reserve hard disconnect for
overlarge request payloads (NBD_CMD_WRITE) or what would require
an overlarge reply payload (NBD_CMD_READ), while any other
command will just receive an error rather than a hard disconnect.

With just one value for 'maximum block size', we have a slight
ambiguity where the server has to decide whether to advertise the
hard disconnect limit (32M) or the maximum size for other
commands, but hopefully the wording chosen here will make it
acceptable for a server that advertises 32M and then permits
clients to try larger WRITE_ZEROES for less traffic (where the
client knows to fall back to smaller requests if the large
request failed with EINVAL); as well as a server that advertises
a larger maximum size (as its actual limit for TRIM and
WRITE_ZEROES) while expecting the client to not exceed 32M
for the denial of service aspect.  The former interpretation
is slightly preferred, especially if a later patch adds a new
NBD_INFO_ field for documenting exact limits for TRIM and
WRITE_ZEROES during NBD_OPT_GO.

Finally, requiring a server to support 32M as its maximum block
size may be a bit large for some setups; when sizes are
advertised, it is reasonable to allow a server with a smaller
explicit limit of 1M.  Rephrase some constraints for easier
reading.

[A later patch will do a no-op reflowing of paragraphs that end
up looking ragged from this patch]

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

doc: Fix requirement on preferred size

Setting the preferred size to >= min(min_size, 4k) doesn't make
sense (a preferred size of 4k when min_size is 64k means that
you get efficient I/O when requesting something smaller than
the minimum granularity?).

Furthermore, there are still raw disks that can perform efficient
I/O on 512-byte sectors (although such hardware is getting rarer
as 4k sectors become more popular), so we can lower the limit
for a reasonable small preferred size.

Fix it by documenting the intended semantics that the preferred
size is >= max(min_size, 512).

Reported-by: Vladimer Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>

doc: Mention FUA flag in more places

The spec is already clear that when the NBD_CMD_FLAG_FUA is in
use, commands that impact the disk don't reply until the action
has landed in permanent storage.  But this information is in a
different section, and someone reading about just the commands
may miss the connection between the flag documentation and the
commands that the flag can affect.  Add wording to NBD_CMD_WRITE
and NBD_CMD_WRITE_ZEROES to fix that.

Although NBD_CMD_TRIM also supports the FUA flag (for specific
implementations where the client knows via external means that
the server will guarantee a reads-as-zero after the trim
completes), the wording under NBD_CMD_TRIM didn't mention timing
to permanent storage, and in general TRIM+FUA makes less sense
between arbitrary client and server (the general definition of
TRIM states that you can't reliably read that area without a
subsequent write, as the read might see zeroes, stale data, or
even unrelated data, so waiting for something you can't read to
land on disk makes no sense).  So that command did not need
the same treatment in this patch.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Enable syslog in the Travis CI build

Add inetd mode testing to the suite

Rearrange things

Having these ordered by short option name makes things easier to figure
out later on.

Document all options supported by nbdtab

Move the assert to the correct location.

We assert that we're only dealing with AF_INET and AF_INET6 here, but it
needs to be true for every address we receive, not just the first one.

Coverity CID #1458574 and 1458573

Allow disabling the NBD_OPT_GO protocol option

For backcompat with nbd-server before commit 2ab3a2d

Fixes: gh-66

Refactor test client

Make inetd mode work again

Merge pull request #72 from eworm-de/misc

nbd-client: free reply on error

nbd-client: improve error output

Fix "make distcheck"

nbd-netlink.h is a new file for this release, which we hadn't added to
Makefile.am yet.

Fix.

Ignore generated files

support/ contains a number of M4 files that we don't want to commit

Don't try to use netlink when we can't

Default to use the netlink interface only when it's available, otherwise
default to ioctl() (and drop the -nonetlink/-L option)

Update docs to reflect reality

Detecting whether the netlink interface is available is just too much
work. Don't bother.

Fix a few GCC warnings

Properly shut down the connection

If a client sends the wrong data, it's usually a good idea to be nice to
them and run gnutls_bye() on them -- even if that doesn't buy is all
that much.

Make -F actually work

We wound up hitting a SIGPIPE, as well as failing before we were testing
for the TEST_EXPECT_ERROR.

Fix both.

Add more ignored stuff

Make this test succeed rather than fail

By adding the -F option, we tell nbd-tester-client that we expect the
setup to fail. Have the be used, rather than XFAIL_TESTS, so that actual
test failures which cause nonzero exit will also fail.

Fix incorrect whitespace

Whoops

Default to switching netlink off, rather than on

Merge branch 'extension-structured-reply'

Qemu 2.11 implemented structured reply, so it is time to promote
this extension to stable.

Merge branch 'origin' into extension-structured-reply

doc: Revert NBD_REPLY_TYPE_BLOCK_STATUS back to 5

Commit 56c77720 renumbered NBD_REPLY_TYPE_BLOCK_STATUS to 3 to
avoid a numbering gap, but it turns out that Virtuozzo already
had a product in production release that was expecting the value
5.  As long as we don't make any changes to the experimental
branch that would not interoperate with the Virtuozzo product,
then keeping the gap is not a problem; if we DO make a change
prior to promoting things to current, we would be better off
renumbering NBD_OPT_SET_META_CONTEXT to a value other than 10
(so that the Virtuozzo product uses opt 10 for its preliminary
implementation, and all modern NBD servers would use opt 11
for the changed layout) - but that's a different decision as
to whether any layout changes are even needed.

Signed-off-by: Eric Blake <eblake@redhat.com>

Improve error handling

Fixes gh-65

Fail if pkg-config is not installed

If the PKG_* macros do not exist, fail immediately with an appropriate error
message, including instructions on what to do.

Migrate to using PKG_CHECK_MODULES

We used AM_PATH_GLIB, a glib-specific macro, to search for glib before.
The modern way is to use PKG_CHECK_MODULES instead.

Migrate to that, so that people don't need to have glib installed when
running autoreconf.

Upgrade things before trying to install stuff

See if the build stops failing on the "travis-latest" images

README.md: point out how to build from git

Fix build without GnuTLS

When GnuTLS was not detected, the Makefile went through a code path that
did not define PROG_NAME. Since the code tries to dereference it anyway,
that resulted in compilation failures.

Fix by defining it. While at it, also define NOTLS, since that seems
prudent.

Fixes gh-63.

server: Consolidate request validation

We had a couple of issues when dealing with bogus requests from
malicious clients, when compared to what we document in the spec
(fortunately, the client is violating protocol by sending these
requests which leaves us a bit more flexibility on our response;
and as we are not crashing, this a quality-of-implementation
rather than a security problem):
- NBD_CMD_TRIM failed with EINVAL instead of EPERM on a read-only image
- range validation for several commands was implicit based on syscall
failures, rather than being filtered out up front (which means we
are dependent on the right errno value, or even risk succeeding at
doing something out of range, although I did not audit all paths)
- At least in the case of NBD_CMD_TRIM, a req->from near 2^64 plus
req->len could overflow into something smaller than client->exportsize
- Duplicating checks everywhere is prone to bitrot

It's easier to consolidate validation up front, so we don't have
multiple copies of range checks, so we don't have to rely on
syscalls catching odd ranges, and so that we are more in line
with what the spec documents.  This in turn requires updating
the testsuite for trim, now that the validation is done in a
different place.

Signed-off-by: Eric Blake <eblake@redhat.com>

doc: Clarify NBD_CMD_READ payload on error

Commit e6c6fb39 clarified that a payload for the reply to NBD_CMD_READ
must not be sent if the error field was set (which matches long-time
existing qemu-nbd behavior; nbd-server was switched to that behavior
at the time; and then just this week nbdkit was also patched to comply).
But it missed another spot in the document, which if read in isolation,
results in the wrong behavior.

Signed-off-by: Eric Blake <eblake@redhat.com>

doc: Update reservation for NBD_REPLY_TYPE_ bits

Use consistent bullet points, and add a reservation for the
BLOCK_STATUS extension.  Note that the reservation is now type
3 rather than 5 to avoid a numbering gap (the choice of 5 in
the BLOCK_STATUS extension pre-dated the decision in the
STRUCTURED_REPLY extension to make all structured error types
include bit 2^15).

Signed-off-by: Eric Blake <eblake@redhat.com>

Merge branch 'master' into extension-structured-reply

doc: Update reserved values for pending extensions

The current state of the BLOCK_STATUS extension proposal no longer
consumes an NBD_FLAG_ bit, but does add a couple of NBD_OPT_ commands
and a new NBD_REP_ reply during handshake, as well as reserving a
new NBD_CMD_FLAG_ bit during transmission.

While at it, fix inconsistent top-level bullets as used in the list
of NBD_OPT_ and NBD_REP_.

The RESIZE extension proposal does not have to skip a bit number in
the NBD_FLAG_ namespace.

Signed-off-by: Eric Blake <eblake@redhat.com>

doc: Update list of current extensions

INFO has been incorporated, and RESIZE should be listed.  Also
mention that BLOCK_SIZE depends on STRUCTURED_REPLY.

Signed-off-by: Eric Blake <eblake@redhat.com>

doc: Fix constant values for NBD_REP_ERR_*

We can't have error values that don't fit in 32 bits :)

Signed-off-by: Eric Blake <eblake@redhat.com>

Merge branch 'master' into extension-structured-reply

Fix a couple more 'non-zero' instances along the way.

More consistent spelling of nonzero

The previous patch touched only docs, but we had more instances of
inconsistencies between 'non-zero' and 'nonzero' in the code base.

Signed-off-by: Eric Blake <eblake@redhat.com>

doc: Consistent spelling of nonzero

We had both 'nonzero' (since 2012) and 'non-zero' (since the
merge of extension-info in 2017); avoid the hyphen for consistency.

Signed-off-by: Eric Blake <eblake@redhat.com>

doc: Explicit mention that 0-length transactions are unspecified

We don't want to forbid 0-length transactions between a client
and server that have some special semantics for it, but we also
don't want to define any particular semantics.  Best is to just
document that portable clients should not attempt it, and that
servers should tolerate clients that do it anyway (whether by a
successful no-op or by an error code, we don't care, as long as
the connection to the client is not killed).

While in the area, mention that a trim request may only cause a
trim action on a subset of the request.

Signed-off-by: Eric Blake <eblake@redhat.com>

Remove unsupported option

When we initially implemented TLS support in nbd-server, originally the
force_tls option was called tlsonly. When this was changed, apparently
we forgot to remove the (now incorrect) tlsonly parameter from the man
page.

Fix.

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

Document the netlink option

Document it the way we want things to behave, rather than the way things
currently are. Up next is actually making current behaviour and
documented behaviour correlate ;-)

client: fix size offset in parsing of response

This fixes flags being set to bogus on 32-bit systems (armv7h),
where sizeof(*uint64_t) != sizeof(uint64_t).

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

structured-reply: Tweak some protocol requirements

Several tweaks noticed while implementing structured reply for qemu:
- Document that errors are possible for NBD_OPT_STRUCTURED_REPLY, and
that non-zero size is an error
- Reformat the paragraph on NBD_CMD_FLAG_DF
- Mention what a client should do if a server sends an unexpected
structured reply
- Document that structured NBD_CMD_READ replies are absolute offsets,
rather than relative to the request offset

Signed-off-by: Eric Blake <eblake@redhat.com>

Merge branch 'master' into extension-structured-reply

Remove symlinks when they're not needed

We create symlinks to the toplevel source files so we don't have to copy
them, but it does mean we leave some cruft in this directory upon "make
clean".

Fix by just adding them to CLEANFILES

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

Improve badges a bit

- Add badge for Travis (yes, it's failing currently)
- Make them all be in markdown format, rather than doing so only for some of them

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

One more

Since the test suite doesn't get compiled by default, we forgot about
this one. Fix.

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

Remove unused variables

gcc -Wall -Werror failed due to a number of unused variables. Remove.

In one case, we really should have been checking that variable, but
didn't. Since after the check there isn't much we can do anymore, drop
the variable and its assignment, and replace it by an if() { error
handling} construct instead.

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

More systemd fixes

We need Before=remote-fs-pre.target, as explained in systemd/3627.

docs: Clarify that some options should be send-once

Some options, like NBD_OPT_STARTTLS or the experimental
NBD_OPT_STRUCTURED_REPLY, only need to be sent once, so
clients should not expect any particular server behavior
if they mistakenly send the option again.  Document this
once up-front, rather than repeating it for every option
that falls in this category.

Signed-off-by: Eric Blake <eblake@redhat.com>

docs: Clarify options that should not have data

The spec indirectly stated that NBD_OPT_LIST should be sent without
data (by virtue of the fact that NBD_REP_ERR_INVALID mentioned
non-zero data length as a reason for failure), but making zero-length
options explicit in the documentation of each option is nicer.
As an exception, make it clear that NBD_OPT_ABORT should succeed at
ending the connection rather than being ignored with an error due
to non-zero length, even though well-behaved clients won't send such
length.

This mirrors existing practice in many implementations, and is done
in a way that can be copied to NBD_OPT_STRUCTURED_REPLY and other
extension options that do not require data.

Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wouter Verhelst <w@uter.be>

Fix HAVE_NETLINK guards

HAVE_NETLINK is always defined, it just is defined to 1 or 0 depending
on whether libnl3 was available.

Fix the guards so they work when libnl3 isn't there, too.

nbd-client: add the ability to use the netlink interface

This adds the ability to setup and tear down an nbd device with the
netlink interface.

Signed-off-by: Josef Bacik <jbacik@fb.com>
(cherry picked from commit c72b53d7d8280ccf27e6316992f2022eb78223d7)
[wouter: update so the patch applies in the face of STARTTLS code]
Signed-off-by: Wouter Verhelst <w@uter.be>

Remove outdated buildbot link

Move badges to a separate page, and add a repology.org badge

Improve --version and --help output

- nbd-client: ensure --version exists, add PACKAGE_BUGREPORT to output, add
  program name to output
- nbd-server: add PACKAGE_BUGREPORT to output

Update list address to reflect the name that was actually assigned

Ignore the minimal (i.e., non-TLS-enabled) version of nbd-client

Don't forget to actually add the configure check

Fixes: 866c2d5752901e8f47c86beb64fea05acf11a825

Ensure that <sys/uio.h> is included when it's needed

Update bugreport address to point to mailinglist (at new address), rather than me

We can't do anything unless and until the network is up and running

Ensure we get shut down properly, too

Now that we have DefaultDependencies=no, we don't have the default
Conflicts=shutdown.target anymore, which means we won't be stopped at
shutdown time.

Add an explicit dependency for that.

Add dependencies for partitions, too

If a device is partitioned, we need to have that depend on the device,
too.

Add DefaultDependencies=no to nbd@ systemd service

This service is involved in early boot, so shouldn't have the default
dependencies...

Remove contraction

vim syntax highlighting gets confused about the extra apostrophe, so
rephrase this sentence so it isn't needed.

Don't define _BSD_SOURCE unconditionally

On older versions of glibc, _BSD_SOURCE was needed to get at the DT_*
macros. On glibc 2.19 and above, however, this is no longer needed; and
on FreeBSD it is actually detrimental since it removes a few symbols
which we do need.

Add a configure.ac check for whether we need _BSD_SOURCE to be set in
order for the DT_* macros to be defined, and only set it in that case.

Destroy the diff file upon client disconnect

When a client disconnects, we need to ensure that the diff file is gone.
Currently, we don't do that, because apparently we forgot.

Fixes #52 on github.

Don't print anything on travis, at all...

Reduce output even further

Don't dist these symlinks

Ensure that syslog doesn't break again

"make distcheck" doesn't have --enable-syslog on by default. Since that
enables a few lines of code that we otherwise don't test, tell automake
to enable that by default for the distcheck build.

That way, we don't release with a broken build again :-)

Fix the LOG_* constants

There is no LOG_WARN, there is only a LOG_WARNING. d'oh.

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

Document the waitfile option

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

Ensure that i is not used uninitialized

Currently, multifile and treefile are mutually exclusive (so this
message cannot be produced in that case), and the only effect this will
have is that the log message will contain a garbage integer value, but
it's not nice.

Ensure i is initialized to 0, so that at least we say that there are no
files in use, in that case.

Coverity CID#1264432

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

Remove extraneous LOG_INFO option

not needed for err_nonfatal

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

Handle failure of open() call

If opening the file for the partition table fails, we can't do much
anyway. However, it makes sense to inform the user of that fact, so that
they won't be confused.

Coverity CID#1298132

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

... and ensure this buffer is null-terminated, too

Coverity CID#1345811

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

Ensure the filename is null-terminated

Coverity CID#1352242

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

Don't overflow s1

s1 is a fixed-size buffer of 150 bytes. Even though that should be
plenty for any kind of error message we might want to produce, it still
makes sense to double-check that we do not overflow this buffer.

Coverity CID#1352244

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

Check that the correct number of bytes was read

It's highly unlikely that we get anything but EOF or sizeof(int), but
not checking that is improper.

Coverity CID#1437702

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

Move the close() call back to where it belongs

In trying to write a fix for CID#1437710, we missed the fact that the
close in the handle_modern_connection() call is necessary for more than
just the failure return of negotiate(). As a result, we now leak a
resource in some cases.

Revert the removal of the close() call, and do it in
handle_modern_connection() anyway.

Coverity CID#1437712

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

Don't deadlock

When trying to commit our diff file in the new wait_file feature, if
something happened to the read or write that we didn't expect, we
currently deadlock. That's not nice.

Instead, log a message, unlock the rwlock, and then return from the
function.

Coverity CID#1437703

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

Code style: add missing space