Merge branch 'extension-structured-reply' into extension-blockstatus

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

Add missing return statement

Coverity CID#1437704

Isn't strictly a problem, but it's nice to be correct.

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

Add missing free()

Coverity CID#1437706

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

Add missing brackets

We want to check whether the NBDC_DO_LIST flag is not set in opts; this
requires that we perform a logical not on the whole expression, rather
than just the first part of it.

Coverity CID#1437709

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

Fix double close

We had two code paths exiting the negotiate() function in error; one
which would close the network, and one which wouldn't.

This is not an ideal situation. Since closing the network socket is
something we'll always need to do anyway, it makes more sense to do it
in the negotiate function rather than in the calling function.

Coverity CID#1437710

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

Fix double free

Coverity CID#1437711

Signed-Off-By: Wouter Verhelst <w@uter.be>

Revert command line additions

We don't want to extend the command line in this way anymore, so revert
the -w option for now.

We may add another way to specify an export from the command line at
some point in the future, but not in this way anymore.

Reduce output, so that travis stops breaking

Merge branch 'master' of https://github.com/danielkucera/nbd

Install GnuTLS for travis/osx

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

Clarify that this isn't an option

clang was complaining that there wasn't an else clause that assigned a
value to addrbits.

Since we should never get there, use assert_not_reached to declare that.

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

Don't forget to initialize socket_closed

We added the socket_closed function, but forgot to initialize it. Fix.

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

Fix the used variable, here

We were using an uninitialized variable. This resulted in undefined behaviour,
and as a result, aggressive optimizing away of that behaviour by clang.

As a result, the test suite was failing.

Fix by using the correct variable

Signed-Off-By: Wouter Verhelst <w@uter.be>

Unbreak the cherry-pick

Fix the location of the nbd-server cflags modification

The "if GNUTLS" conditional was nested inside an "if CLIENT"
conditional. That made sense for the client bits, but not for the server
bits.

Move those out into their rightful place.

Add missing "return true"

Spotted-By: Greg Menke <gregory.d.menke@nasa.gov>

implemented waiting for file

If we expect an export file to appear and know its size, we can
temporarily save writes into diff file. When the file appears
we commit all writes into it and switch writes to export file.

This mode can be useful when live migrating VM between hypervisors:
We start mirroring writes from source HV to destination despite
the destination storage doesn't exist yet. Then we can start
copying the source storage (ZFS, LVM, ...) to destination and
when it appears on destination, cached writes are commited to it.

Merge commit 'f673479c3b8a104cdd5c5561f3ae55c9dc486ecd'

We'd rebase, but we've already (force-)pushed this, let's not overdo it.

hate mess.

Merge branch 'extension-info'

For real this time, without losing a few commits that were still on top

Handle socket errors better

Previously, when reading from or writing to a socket failed, we would
handle errors by simply producing an error message and exiting the
program. While that works, it does not allow for the postrun command,
specified in the configuration file, to run.

Fix this by adding a return value to readit/writeit-like functions, and
handling nonzero return values in the socket_read()/socket_write()
function, which then call a cleanup function that does the same exit
(but which will allow for doing the necessary postrun stuff).

Fixes #47 on Github

Merge branch 'extension-info'

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

Don't forget to read the length of the request

We read out the NBD_OPT_INFO packet's length as the length of the name
(which follows it immediately), rather than the length of the packet.

Add a second read to fix that.

While at it, add a sanity check for the length of both, and send back an
NBD_REP_ERR_INVALID if it fails.

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

Regenerate certificate

Although the configuration file specified a validity of 3650 days (i.e.,
almost 10 years), the actual certificate in this directory was generated
with a certificate validity of 1 year. It expired on april 19th, 2017
(i.e., 5 days ago), causing the test suite to fail from now on.

Fix by regenerating the certificate (not the keys) of the certificate
with the actual configuration in this directory.

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

Follow the spec a bit more.

When a TLS-only export is used, we should send TLS_REQD, not UNKNOWN. We
are allowed, however, to also send TLS_REQD for unknown exports (but
we were not going to do that).

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

Implement NBD_OPT_GO in the client

We don't (yet) use any information apart from NBD_INFO_EXPORT, and we
don't call NBD_OPT_INFO, either, since we need neither; however, write
it in a manner that makes it possible and easy to add said functionality
in the future.

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

Remove warning

%llu is nonstandard and not guaranteed to match a uint64_t variable.
PRIu64 is.

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

Allow for not passing any data

It's sometimes useful to call this function so that a header is sent
out, without actually sending the data (yet), as we already allow in the
server.

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

Merge branch 'master' into extension-info

Print the description properly

Previously, when a server sent a description along with the name, we
would try to parse part of the description as the magic number of the
next reply, and fail (d'oh).

Fix parsing of the NBD_REP_SERVER reply by properly parsing and
displaying the contents of the message.

Fix typo

Swap options and option name

When trying to reply to a client with certain information, it's useful
if we don't need to read the whole reply but can handle it by reading
one option, handling it, reading the next one, etc etc. This requires
that the server is told which export to deal with before getting the
option names, however.

As specified, that isn't the case, so this would require the server to
read in the whole request before it can start processing it.

Swap options around to make handling the INFO/GO messages easier to do.

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

Merge branch 'master' into extension-info

Stop open-coding request sending

We have a "send_reply" function in the server which makes it easier to
send a negotiation reply to the client. So why don't we implement the
same thing for the client, too?

Add it, so that sending a request should become easier.

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

Move writeit to cliserv

A function to write until success is useful regardless of whether we're
in the client or in the server.

As such, move it to cliserv so we can use it from both.

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

Implement NBD_OPT_INFO and NBD_OPT_GO

Move more initialisation into commit_client

All callers were doing the same preparatory initialisation. That's a bit
silly; better to move it into the function instead.

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

Allow for sending the data outside of the send_reply() function

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

Turn various fatal errors into return values

Previously, we would err() out of a function whenever things went south.
That's fine for NBD_OPT_EXPORT_NAME, but not for NBD_OPT_GO, which needs
to deal with things differently.

Turn each and every one of those err() calls into an err_nonfatal, and
change the return value of the function.

While at it, harmonize the function return signatures into booleans
everywhere.

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

Don't forget to return a value from commit_client

Upon exiting the function with successful results, we shouldn't forget
to actually return true...

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

Handle commit_client failures

The commit_client() function is set up to return false when things fail.
We should actually handle that situation, then, rather than ignoring it.

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

Move send_export_info up

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

Move copyonwrite_prepare up

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

Move setupexport up

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

Move do_run up

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

Move set_peername up

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

Centralize what needs to be done before exporting

Previously, we ran everything that needed to be done before exporting
for a particular client (e.g., checking access rights, opening files,
running scripts, ...) in various locations all over the place. This
works, but it assumes NBD_OPT_EXPORT_NAME usage only, because it sends
the sizes and flags etc unconditionally after we've left the
negotiation() function.

This works for NBD_OPT_EXPORT_NAME, but not for NBD_OPT_GO, which needs
to send a negotiation acknowledgement before formally closing the
negotiation phase. As such, rearrange things so that they're all in one
function, commit_client, which does all the setup, and call that
function before returning from the negotiation() function.

This new function wants to call a number of other functions that need to
be moved up in the nbd-server file; but we'll do that after this commit
so as to not muddle the diff with irrelevant fluff. Add forward
declarations to them for now.

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

Add missing backticks

Merge branch 'master' into extension-info

Remove incorrect statements

STARTTLS has been implemented by the reference implementation now, too.

doc: Fix some minor issues

Fix a grammar nit, and resolve a conflict between two extensions
picking the same command number (BLOCK_STATUS is the older proposal,
so it keeps command 7, RESIZE gets command 8).

Signed-off-by: Eric Blake <eblake@redhat.com>
[added in missing typo fix]
Signed-off-by: Wouter Verhelst <w@uter.be>

build: Fix build with older gcc

gcc 4.4.7 (hello RHEL 6) complains about redefinition of typedefs,
as in:

gcc -std=gnu99 -DHAVE_CONFIG_H -I.  -DSYSCONFDIR='"/usr/local/etc"'  -g -O2  -g -O2 -MT nbd_client-buffer.o -MD -MP -MF .deps/nbd_client-buffer.Tpo -c -o nbd_client-buffer.o `test -f 'buffer.c' || echo './'`buffer.c
buffer.c:39: error: redefinition of typedef ‘buffer_t’
buffer.h:31: note: previous declaration of ‘buffer_t’ was here

The .c file only has to declare the struct being typedef'd, not
repeat the typedef declaration.

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

Use AM_CONDITIONAL rather than AC_SUBST

AC_SUBST'ing the nbd-client program worked when we didn't need to
compile the client twice. Now that we do, this makes us try to compile
the minimal (i.e., non-TLS) client even if we're on a system where the
client can't be compiled. This is obviously wrong.

Fix it by using AM_CONDITIONAL a second time, and nesting the whole
GnuTLS conditional below that.

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

Don't seek() in files without holding a lock.

The introduction of multithread handling of requests failed to consider
what happens with copyonwrite enabled. We would seek, then read (or
write), a file.

If we have copyonwrite enabled, this fails badly in that two threads can
race against eachother for the current file pointer. Instead, we should
use pread() or pwrite() to read from offset without relying on the
current file offset, as explained in #43.

Remove the myseek() function to ensure we don't make use of it anymore,
and fix up all callers.

The only exception where we can still use lseek() safely is in the
open_treefile() function, since there we hold a mutex and hence we can't
race against other threads.

TODO: test whether this fixes the problem.

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

Add numbers for resize extension

Rename request in line with the kernel

Closes #45

NBD_REP_ERR_UNSUP not EINVAL for options

Signed-off-by: Alex Bligh <alex@alex.org.uk>

Make returned default status context dependent

Signed-off-by: Alex Bligh <alex@alex.org.uk>

Re-sort options in this man page

The options are supposed to be alphabetically ranked, but they weren't
anymore. Fix that.

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