NetworkBlockDevice / nbd

Network Block Device
GNU General Public License v2.0
459 stars 119 forks source link

inetd mode fix #76

Closed fstirlitz closed 6 years ago

fstirlitz commented 6 years ago

This series of patches restores proper functioning of the inetd mode in nbd-server. It also adds inetd mode testing to the test suite. Merging it will resolve #75.

Implementing the latter part required some fairly invasive refactoring of the test client. The testing functions are no longer involved with establishing the transport socket connection; they are handed the socket file descriptor already set up, although they still need to perform NBD protocol negotiation. The name of the protocol-negotiating function was kept as setup_connection_common, although it is no longer called from the setup_*_connection family of functions.

yoe commented 6 years ago

On Tue, Apr 03, 2018 at 02:22:11PM +0000, Felix S. wrote:

@fstirlitz commented on this pull request.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

In tests/run/simple_test:

@@ -24,8 +24,6 @@ cleanup() { if [ ! -z "$PID" ] then kill $PID || true

  • else
  • echo "E: Could not clean up!"

Testing in inetd mode has nbd-tester-client spawn nbd-server on its own, instead of having it launched by the shell. The former doesn't return the latter's PID back to the testing script, so there's nothing to set the PID variable to, and the script would otherwise complain here.

Ah, yes, of course; didn't think of that.

I guess theoretically if the script could somehow open a socket pair, it could then pass appropriate file descriptors to nbd-tester-client and nbd-server and obtain a PID of the latter, but I don't see any way to accomplish this portably.

Nah, not worth it. I suppose refactoring the script could work, but meh.

-- Could you people please use IRC like normal people?!?

-- Amaya Rodrigo Sastre, trying to quiet down the buzz in the DebConf 2008 Hacklab

yoe commented 6 years ago

On Tue, Apr 03, 2018 at 02:44:24PM +0000, Felix S. wrote:

@fstirlitz commented on this pull request.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

In tests/run/nbd-tester-client.c:

@@ -1743,6 +1772,8 @@ int main(int argc, char **argv) case 'f': testflags |= TEST_FLUSH; break;

  • case 'I':
  • goto inetd_mode;

I'm pretty sure this is a violation of the POSIX standard. (And the C standard as well: the argv argument to getopt is a char *const [].) Of course, standard violations do happen in the real world, but it seems to work fine anyway with GNU's version of getopt, which indeed does such a thing. I guess I can change it, though.

Thanks.

(As for hairyness, it's not like it wasn't hairy before. See previous commit.)

Well, yeah, but that doesn't mean we should make it worse :-)

-- Could you people please use IRC like normal people?!?

-- Amaya Rodrigo Sastre, trying to quiet down the buzz in the DebConf 2008 Hacklab

fstirlitz commented 6 years ago

Might as well cherry-pick my latest commit which handles execvp failure.