inetd mode is broken

[fstirlitz]

Whenever nbd-server is launched in inetd mode (passing 0 as the port on the command line), it immediately exits, after sending these two messages to syslog:

getsockname failed: Bad file descriptor
Failed to set peername

I managed to make it work by applying this patch (against 3.17):

--- nbd-3.17/nbd-server.c
+++ nbd-3.17/nbd-server.c
@@ -3538,19 +3538,6 @@ int main(int argc, char *argv[]) {

        if(serve) {
                g_array_append_val(servers, *serve);
-
-               if(strcmp(genconf.modernport, "0")==0) {
-#ifndef ISSERVER
-                       err("inetd mode requires syslog");
-#endif
-                       CLIENT* client = g_malloc(sizeof(CLIENT));
-                       client->net = -1;
-                       if(!commit_client(client, serve)) {
-                               exit(EXIT_FAILURE);
-                       }
-                       mainloop_threaded(client);
-                       return 0;
-               }
        }

        if(!servers || !servers->len) {
@@ -3594,5 +3581,18 @@ int main(int argc, char *argv[]) {
 #endif
                                        ));
 #endif
+
+       if(strcmp(genconf.modernport, "0")==0) {
+#ifndef ISSERVER
+               err("inetd mode requires syslog");
+#endif
+               CLIENT* client = negotiate(0, servers, &genconf);
+               if(!client) {
+                       exit(EXIT_FAILURE);
+               }
+               mainloop_threaded(client);
+               return 0;
+       }
+
        serveloop(servers, &genconf);
 }

But I feel this patch may be too heavy-handed, so I'm not submitting it as a pull request.

[yoe]

I think it actually makes sense.

I don't see why you moved this from inside the "if (serve)" thing to outside, though. Care to comment?

Also, inetd mode has broken several times over the years, mainly because our test suite doesn't test for it. I'd be more inclined to accept a patch fixing it if it also extended the test suite to test for it. Otherwise it will just break again in the future, and that would be a waste of effort.

[fstirlitz]

I think it actually makes sense.

...'it' being...?

I don't see why you moved this from inside the "if (serve)" thing to outside, though. Care to comment?

mainloop_threaded working depends on tpool being set up, while negotiation probably depends on GnuTLS being initialised (I have only skimmed the code); both things are done further down in main. I didn't want to duplicate any initialisation code; just moving this block down seemed like the simplest way to fix this.

I'd be more inclined to accept a patch fixing it if it also extended the test suite to test for it.

Okay. Do you think it should be done by extending nbd-tester-client to launch nbd-server by itself, writing a stub inetd-like program, or adding a dependency on inetd/ncat/socat?

[yoe]

On Sat, Mar 31, 2018 at 09:45:16AM +0000, Felix S. wrote:

I think it actually makes sense.

...'it' being...?

The patch you sent :-)

I don't see why you moved this from inside the "if (serve)" thing to
outside, though. Care to comment?

mainloop_threaded working depends on tpool being set up, while negotiation probably depends on GnuTLS being initialised (I have only skimmed the code); both things are done further down in main. I didn't want to duplicate any initialisation code; just moving this block down seemed like the simplest way to fix this.

Ah, yes.

Thinking about it a bit more though, I guess I wasn't thinking straight.

The idea of inetd mode has always been that it would only support config file mode, because originally you could only do the oldstyle protocol over inetd. Hence the "if (serve)" bit there, because otherwise there was no way to figure out which export we're trying to do here.

Since we dropped oldstyle now however, that doesn't make as much sense anymore, so yeah, your patch seems to be doing the right thing.

I'd be more inclined to accept a patch fixing it if it also extended the
test suite to test for it.

Okay. Do you think it should be done by extending nbd-tester-client to launch nbd-server by itself, writing a stub inetd-like program, or adding a dependency on inetd/ncat/socat?

I think the former seems like the best way to go; there are way too many inetd implementations, and depending on exactly one will very likely cause problems down the line.

This could be something along the lines of:

nbd-tester-client --inetd "$(top_srcdir)/nbd-server "

or some such; I could imagine that an option with a -- in the command line would work too and might be easier to do depending on implementation. The important part here would be to ensure that nbd-server arguments are specified on the nbd-tester-client command line.

Thanks,

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

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