date:20161017

[Nbd] [PATCH v2 1/6] build: Silence autogen.sh warnings

2016-10-17 Thread Eric Blake

Starting from a fresh git checkout, running ./autogen.sh gives a
couple of warnings on my Fedora 24 build tools, one from libtool:

libtoolize: Consider adding '-I support' to ACLOCAL_AMFLAGS in Makefile.am.

and one from automake:

tests/run/Makefile.am:4: warning: source file '$(top_srcdir)/cliserv.c' is in a 
subdirectory,
tests/run/Makefile.am:4: but option 'subdir-objects' is disabled
automake: warning: possible forward-incompatibility.
automake: At least a source file is in a subdirectory, but the 'subdir-objects'
automake: automake option hasn't been enabled.  For now, the corresponding 
output
automake: object file(s) will be placed in the top-level directory.  However,
automake: this behaviour will change in future Automake versions: they will
automake: unconditionally cause object files to be placed in the same 
subdirectory
automake: of the corresponding sources.
automake: You are advised to start using 'subdir-objects' option throughout your
automake: project, to avoid future incompatibilities.

Following the advice almost works, except that automake 1.15 still
has a nasty bug (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=13928)
where use of $(foo) in a _SOURCES variable coupled with subdir-objects
creates a directory with a literal name $(foo) rather than the intended
name.  But as long as we only care about $(srcdir) (or parent
directories), the solution is to just not use $(srcdir) in _SOURCES,
and instead open-code the traversal to the desired files.

I also noticed that the build was already leaving behind an untracked
manpage.log file, in addition to the new .dirstamp witness file created
by our new use of subdir-objects.

Signed-off-by: Eric Blake 
---
 .gitignore| 2 ++
 Makefile.am   | 1 +
 configure.ac  | 2 +-
 tests/run/Makefile.am | 2 +-
 4 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index b8163e0..b005b11 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,4 +1,5 @@
 .deps
+.dirstamp
 Makefile
 autom4te.cache
 autoscan.log
@@ -38,6 +39,7 @@ install-sh
 configure
 man/*.sh
 man/*.sh.in
+man/manpage.log
 make-integrityhuge
 nbd-trdump
 missing
diff --git a/Makefile.am b/Makefile.am
index 32774e3..c1740d6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,3 +1,4 @@
+ACLOCAL_AMFLAGS = -I support
 SUBDIRS = . man doc tests systemd gznbd
 bin_PROGRAMS = nbd-server nbd-trdump
 sbin_PROGRAMS = @NBD_CLIENT_NAME@
diff --git a/configure.ac b/configure.ac
index 83e4f91..ce225a6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -11,7 +11,7 @@ m4_define([serial_tests], [
   awk '{split ($NF,a,"."); if (a[1] == 1 && a[2] >= 12) { print 
"serial-tests" }}'
   ])
 ])
-AM_INIT_AUTOMAKE(foreign dist-xz serial_tests)
+AM_INIT_AUTOMAKE(foreign dist-xz serial_tests subdir-objects)
 AM_MAINTAINER_MODE([enable])
 AC_CONFIG_MACRO_DIR([support])
 LT_INIT
diff --git a/tests/run/Makefile.am b/tests/run/Makefile.am
index c9cfa8f..b790c3f 100644
--- a/tests/run/Makefile.am
+++ b/tests/run/Makefile.am
@@ -1,7 +1,7 @@
 TESTS_ENVIRONMENT=$(srcdir)/simple_test
 TESTS = cfg1 cfgmulti cfgnew cfgsize write flush integrity dirconfig list 
rowrite tree rotree unix integrityhuge
 check_PROGRAMS = nbd-tester-client
-nbd_tester_client_SOURCES = nbd-tester-client.c $(top_srcdir)/cliserv.h 
$(top_srcdir)/netdb-compat.h $(top_srcdir)/cliserv.c
+nbd_tester_client_SOURCES = nbd-tester-client.c ../../cliserv.h 
../../netdb-compat.h ../../cliserv.c
 nbd_tester_client_CFLAGS = @CFLAGS@ @GLIB_CFLAGS@
 nbd_tester_client_CPPFLAGS = -I$(top_srcdir)
 nbd_tester_client_LDADD = @GLIB_LIBS@
-- 
2.7.4


--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

[Nbd] [PATCH v2 2/6] server: Fix botched strlen computation of error message

2016-10-17 Thread Eric Blake

Commit 3b80382 tried to make it easy for the server to send an
error message whose length was determined by strlen(), but ended
up sending a length of UINT32_MAX, causing clients to either
hang up (reply too large) or wait for nearly 4G of data that was
never coming.

Signed-off-by: Eric Blake 
---
 nbd-server.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/nbd-server.c b/nbd-server.c
index 25c335b..d0c6fa6 100644
--- a/nbd-server.c
+++ b/nbd-server.c
@@ -1321,11 +1321,12 @@ static void send_reply(CLIENT* client, uint32_t opt, 
uint32_t reply_type, ssize_
htonl(reply_type),
htonl(datasize),
};
+   if(datasize < 0) {
+   datasize = strlen((char*)data);
+   header.datasize = htonl(datasize);
+   }
socket_write(client, , sizeof(header));
if(datasize != 0) {
-   if(datasize < 0) {
-   datasize = strlen((char*)data);
-   }
socket_write(client, data, datasize);
}
 }
-- 
2.7.4


--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

[Nbd] [PATCH v2 3/6] server: Swap argument order in consume()

2016-10-17 Thread Eric Blake

The signature of consume() threw me off.  Good design says that if
you are going to have paired parameters (buf and bufsize), you
generally want them adjacent, not separated by an unrelated parameter
(len).  Move len to be first, adjusting all callers.

Signed-off-by: Eric Blake 
---
 nbd-server.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/nbd-server.c b/nbd-server.c
index d0c6fa6..c93a9d8 100644
--- a/nbd-server.c
+++ b/nbd-server.c
@@ -359,12 +359,12 @@ static void socket_read(CLIENT* client, void *buf, size_t 
len) {
 /**
  * Consume data from a socket that we don't want
  *
- * @param f a file descriptor
- * @param buf a buffer
+ * @param c the client to read from
  * @param len the number of bytes to consume
+ * @param buf a buffer
  * @param bufsiz the size of the buffer
  **/
-static inline void consume(CLIENT* c, void * buf, size_t len, size_t bufsiz) {
+static inline void consume(CLIENT* c, size_t len, void * buf, size_t bufsiz) {
size_t curlen;
while (len>0) {
curlen = (len>bufsiz)?bufsiz:len;
@@ -1853,14 +1853,14 @@ int mainloop(CLIENT *client) {
(client->server->flags & F_AUTOREADONLY)) {
DEBUG("[WRITE to READONLY!]");
ERROR(client, reply, EPERM);
-   consume(client, buf, len-currlen, 
BUFSIZE);
+   consume(client, len-currlen, buf, 
BUFSIZE);
continue;
}
if (expwrite(request.from, buf, currlen, client,
 request.type & NBD_CMD_FLAG_FUA)) {
DEBUG("Write failed: %m" );
ERROR(client, reply, errno);
-   consume(client, buf, len-currlen, 
BUFSIZE);
+   consume(client, len-currlen, buf, 
BUFSIZE);
continue;
}
len -= currlen;
-- 
2.7.4


--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

[Nbd] [PATCH v2 6/6] server: Read client's TLS length data before next option

2016-10-17 Thread Eric Blake

Any client attempting to probe support for a new option, such as
NBD_OPT_STARTTLS or NBD_OPT_GO, with plans to do a graceful
fallback to older methods, will fail in its attempt if the server
does not consume the length field and potential payload of the
unrecognized (or rejected) option, because the server will then
be reading out of sync and not see the client's magic for the
next option.  While it is true that sane clients are unlikely to
send more than one NBD_OPT_STARTTLS, and thus never trigger some
of the paths in this patch, it is still better to be robust for
all clients.

Furthermore, even if the server requires TLS, and rejects all but
NBD_OPT_STARTTLS as the first valid option, it should still honor
NBD_OPT_ABORT.

Signed-off-by: Eric Blake 
---
 nbd-server.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/nbd-server.c b/nbd-server.c
index 4b0692d..d5b2013 100644
--- a/nbd-server.c
+++ b/nbd-server.c
@@ -1507,6 +1507,11 @@ CLIENT* negotiate(int net, GArray* servers, const gchar* 
tlsdir) {
// so must do hard close
goto hard_close;
}
+   if(opt == NBD_OPT_ABORT) {
+   // handled below
+   break;
+   }
+   consume_len(client);
send_reply(client, opt, NBD_REP_ERR_TLS_REQD, -1, "TLS 
is required on this server");
continue;
}
@@ -1529,11 +1534,14 @@ CLIENT* negotiate(int net, GArray* servers, const 
gchar* tlsdir) {
break;
case NBD_OPT_STARTTLS:
if(client->tls_session != NULL) {
+   consume_len(client);
send_reply(client, opt, NBD_REP_ERR_INVALID, 
-1, "Invalid STARTTLS request: TLS has already been negotiated!");
continue;
}
if(tlsdir == NULL) {
+   consume_len(client);
send_reply(client, opt, NBD_REP_ERR_POLICY, -1, 
"TLS not allowed on this server");
+   continue;
}
if(handle_starttls(client, opt, servers, cflags, 
tlsdir) == NULL) {
// can't recover from failed TLS negotiation.
-- 
2.7.4


--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

[Nbd] [PATCH v2 4/6] server: Read client's unknown option length before next option

2016-10-17 Thread Eric Blake

Any client attempting to probe support for a new option, such as
NBD_OPT_STARTTLS or NBD_OPT_GO, with plans to do a graceful
fallback to older methods, will fail in its attempt if the server
does not consume the length field and potential payload of the
unrecognized (or rejected) option, because the server will then
be reading out of sync and not see the client's magic for the
next option.  This bug has been latent in the reference
server since commit 626c2a3 in 2012, even though it is EXACTLY
the bug that NBD_FLAG_FIXED_NEWSTYLE was designed to prevent.

The only reason it has been buggy for so long is that it has
taken us this long to finally want to implement clients that use
a new option.

This patch fixes only the portion of the server that has been
previously released, to make backports easier. The new code for
handling TLS also needs fixing, in the next patch.

Signed-off-by: Eric Blake 
---
 nbd-server.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/nbd-server.c b/nbd-server.c
index c93a9d8..4b0692d 100644
--- a/nbd-server.c
+++ b/nbd-server.c
@@ -373,6 +373,21 @@ static inline void consume(CLIENT* c, size_t len, void * 
buf, size_t bufsiz) {
}
 }

+/**
+ * Consume a length field and corresponding payload that we don't want
+ *
+ * @param c the client to read from
+ **/
+static inline void consume_len(CLIENT* c) {
+   uint32_t len;
+   char buf[1024];
+
+   socket_read(c, , sizeof(len));
+   len = ntohl(len);
+   consume(c, len, buf, sizeof(buf));
+}
+
+
 static void socket_write(CLIENT* client, void *buf, size_t len) {
g_assert(client->socket_write != NULL);
client->socket_write(client, buf, len);
@@ -1525,6 +1540,7 @@ CLIENT* negotiate(int net, GArray* servers, const gchar* 
tlsdir) {
goto hard_close;
}
default:
+   consume_len(client);
send_reply(client, opt, NBD_REP_ERR_UNSUP, -1, "The 
given option is unknown to this server implementation");
break;
}
-- 
2.7.4


--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

Re: [Nbd] [PATCH] server: Read client's length data before next option

2016-10-17 Thread Wouter Verhelst

On Sun, Oct 16, 2016 at 02:19:54PM +0100, Alex Bligh wrote:
> 
> > On 14 Oct 2016, at 22:11, Eric Blake  wrote:
> > 
> > Given that we have 4 years of buggy servers that will fail to react
> > correctly to NBD_OPT_GO and friends, is it worth enhancing the docs to
> > suggest that a robust client should be prepared for (buggy) servers that
> > mistakenly hang up after sending an NBD_OPT_ERR_UNSUP, and try
> > reconnecting to the server while avoiding the command that failed on the
> > previous run?  Eventually, buggy servers will disappear, so we can't
> > mandate that clients take that extra step, but since it is a very common
> > problem at the present, it might help client implementations to be aware
> > of it.  Then again, most client implementors read this list, so
> > documenting it in the protocol may be overkill.
> 
> I think the answer there is 'fix the server' rather than work
> around it in the client.

Except we're talking here about "known buggy in-the-wild
implementations". Yes, you can fix the newer versions of the known buggy
implementations, but that doesn't help what's already out there.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
   people in the world who think they really understand all of its rules,
   and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

Re: [Nbd] [PATCH] build: Silence autogen.sh warnings

2016-10-17 Thread Eric Blake

On 10/14/2016 03:19 PM, Eric Blake wrote:
> Starting from a fresh git checkout, running ./autogen.sh gives a
> couple of warnings on my Fedora 24 build tools, one from libtool:
> 
> libtoolize: Consider adding '-I support' to ACLOCAL_AMFLAGS in Makefile.am.
> 
> and one from automake:
> 
> tests/run/Makefile.am:4: warning: source file '$(top_srcdir)/cliserv.c' is in 
> a subdirectory,
> tests/run/Makefile.am:4: but option 'subdir-objects' is disabled
> automake: warning: possible forward-incompatibility.
> automake: At least a source file is in a subdirectory, but the 
> 'subdir-objects'
> automake: automake option hasn't been enabled.  For now, the corresponding 
> output
> automake: object file(s) will be placed in the top-level directory.  However,
> automake: this behaviour will change in future Automake versions: they will
> automake: unconditionally cause object files to be placed in the same 
> subdirectory
> automake: of the corresponding sources.
> automake: You are advised to start using 'subdir-objects' option throughout 
> your
> automake: project, to avoid future incompatibilities.
> 
> Following the advice doesn't break anything, so let's do it.

Except my patch did break something. NACK; this will need v2.

> 
> I also noticed that the build leaves behind an untracked file.
> 
> Signed-off-by: Eric Blake 
> ---
>  .gitignore   | 1 +
>  Makefile.am  | 1 +
>  configure.ac | 2 +-
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/.gitignore b/.gitignore
> index b8163e0..a13897a 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -38,6 +38,7 @@ install-sh
>  configure
>  man/*.sh
>  man/*.sh.in
> +man/manpage.log
>  make-integrityhuge
>  nbd-trdump
>  missing
> diff --git a/Makefile.am b/Makefile.am
> index 32774e3..c1740d6 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1,3 +1,4 @@
> +ACLOCAL_AMFLAGS = -I support
>  SUBDIRS = . man doc tests systemd gznbd
>  bin_PROGRAMS = nbd-server nbd-trdump
>  sbin_PROGRAMS = @NBD_CLIENT_NAME@
> diff --git a/configure.ac b/configure.ac
> index 83e4f91..ce225a6 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -11,7 +11,7 @@ m4_define([serial_tests], [
>awk '{split ($NF,a,"."); if (a[1] == 1 && a[2] >= 12) { print 
> "serial-tests" }}'
>])
>  ])
> -AM_INIT_AUTOMAKE(foreign dist-xz serial_tests)
> +AM_INIT_AUTOMAKE(foreign dist-xz serial_tests subdir-objects)
>  AM_MAINTAINER_MODE([enable])
>  AC_CONFIG_MACRO_DIR([support])
>  LT_INIT
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

Re: [Nbd] build failure

2016-10-17 Thread Eric Blake

On 10/15/2016 01:46 PM, Wouter Verhelst wrote:
> On Fri, Oct 14, 2016 at 03:11:55PM -0500, Eric Blake wrote:
>> On 10/14/2016 01:32 PM, Wouter Verhelst wrote:
>>> On Thu, Oct 13, 2016 at 05:33:18PM -0500, Eric Blake wrote:
 I'm getting this failure when trying to build NBD, as part of
 'autoreconf -vfi':

 configure.ac:248: error: required file 'systemd/n...@.service.sh.in' not
 found
>>>
>>> That's why we have autogen.sh ;-)
>>
>> Okay, that got me further, but there's still warnings about improper
>> automake usage, and then still a fatal build failure during make:
>>
>> make[3]: Entering directory '/home/eblake/nbd/tests/run'
>> Makefile:403: ../../.deps/nbd_tester_client-cliserv.Po: No such file or
>> directory
>> make[3]: *** No rule to make target
>> '../../.deps/nbd_tester_client-cliserv.Po'.  Stop.
> 
> That shouldn't happen, unless you've done something terribly wrong.

Oh, I see what happened. My attempted patch to silence automake warnings
is not quite right, because now I get:
$ find -name nbd_tester_client\*
./tests/run/$(top_srcdir)/.deps/nbd_tester_client-cliserv.Po
./tests/run/.deps/nbd_tester_client-nbd-tester-client.Po

Whoops - that literal '$(top_srcdir)' directory name is a (known)
automake issue, so I'll have to fix my patch.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

[Nbd] [PATCH v2 1/6] build: Silence autogen.sh warnings

[Nbd] [PATCH v2 2/6] server: Fix botched strlen computation of error message

[Nbd] [PATCH v2 3/6] server: Swap argument order in consume()

[Nbd] [PATCH v2 6/6] server: Read client's TLS length data before next option

[Nbd] [PATCH v2 4/6] server: Read client's unknown option length before next option

Re: [Nbd] [PATCH] server: Read client's length data before next option

Re: [Nbd] [PATCH] build: Silence autogen.sh warnings

Re: [Nbd] build failure

8 matches

Site Navigation

Mail list logo

Footer information