Re: [PATCH] qemu-nbd: Restore "qemu-nbd -v --fork" output
On 8/24/23 22:03, Eric Blake wrote: Closing stderr earlier is good for daemonized qemu-nbd under ssh earlier, but breaks the case where -v is being used to track what is happening in the server, as in iotest 233. When we know we are verbose, we do NOT want qemu_daemon to close stderr. For management purposes, we still need to temporarily override the daemon child's stderr with the pipe to the parent until after the pid file is created; but since qemu_daemon would normally set stdout to /dev/null had we not been verbose, we can use stdout as a place to stash our original stderr. Thus, whether normal or vebose, when the management handoff is complete, copying stdout back to stderr does the right thing for the rest of the life of the daemon child. Note that while the error messages expected by iotest 233 are now restored, the change in file descriptors means they now show up earlier in the testsuite output. Reported-by: Kevin Wolf CC: Denis V. Lunev CC: qemu-sta...@nongnu.org Fixes: 5c56dd27a2 ("qemu-nbd: fix regression with qemu-nbd --fork run over ssh") Signed-off-by: Eric Blake --- qemu-nbd.c | 21 - tests/qemu-iotests/233.out | 20 ++-- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/qemu-nbd.c b/qemu-nbd.c index aaccaa33184..a105094fb17 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -944,9 +944,24 @@ int main(int argc, char **argv) close(stderr_fd[0]); -ret = qemu_daemon(1, 0); +ret = qemu_daemon(1, verbose); saved_errno = errno;/* dup2 will overwrite error below */ +if (verbose) { +/* We want stdin at /dev/null when qemu_daemon didn't do it */ +stdin = freopen("/dev/null", "r", stdin); +if (stdin == NULL) { +error_report("Failed to redirect stdin: %s", + strerror(errno)); +exit(EXIT_FAILURE); +} +/* To keep the parent's stderr alive, copy it to stdout */ +if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) { +error_report("Failed to redirect stdout: %s", + strerror(errno)); +exit(EXIT_FAILURE); +} +} /* Temporarily redirect stderr to the parent's pipe... */ if (dup2(stderr_fd[1], STDERR_FILENO) < 0) { char str[256]; @@ -1180,6 +1195,10 @@ int main(int argc, char **argv) } if (fork_process) { +/* + * See above. If verbose is false, stdout is /dev/null (thanks + * to qemu_daemon); otherwise, stdout is the parent's stderr. + */ if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) { error_report("Could not set stderr to /dev/null: %s", strerror(errno)); diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out index 237c82767ea..b09a197020a 100644 --- a/tests/qemu-iotests/233.out +++ b/tests/qemu-iotests/233.out @@ -41,8 +41,10 @@ exports available: 1 min block: 1 == check TLS fail over TCP with mismatched hostname == +qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort qemu-img: Could not open 'driver=nbd,host=localhost,port=PORT,tls-creds=tls0': Certificate does not match the hostname localhost qemu-nbd: Certificate does not match the hostname localhost +qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort == check TLS works over TCP with mismatched hostname and override == image: nbd://localhost:PORT @@ -55,7 +57,9 @@ exports available: 1 min block: 1 == check TLS with different CA fails == +qemu-nbd: option negotiation failed: Verify failed: No certificate was found. qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': The certificate hasn't got a known issuer +qemu-nbd: option negotiation failed: Verify failed: No certificate was found. qemu-nbd: The certificate hasn't got a known issuer == perform I/O over TLS == @@ -67,11 +71,15 @@ read 1048576/1048576 bytes at offset 1048576 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == check TLS with authorization == +qemu-nbd: option negotiation failed: TLS x509 authz check for C=South Pacific,L=R'lyeh,O=Cthulhu Dark Lord Enterprises client1,CN=localhost is denied qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort +qemu-nbd: option negotiation failed: TLS x509 authz check for C=South Pacific,L=R'lyeh,O=Cthulhu Dark Lord Enterprises client3,CN=localhost is denied qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option
[PATCH] qemu-nbd: Restore "qemu-nbd -v --fork" output
Closing stderr earlier is good for daemonized qemu-nbd under ssh earlier, but breaks the case where -v is being used to track what is happening in the server, as in iotest 233. When we know we are verbose, we do NOT want qemu_daemon to close stderr. For management purposes, we still need to temporarily override the daemon child's stderr with the pipe to the parent until after the pid file is created; but since qemu_daemon would normally set stdout to /dev/null had we not been verbose, we can use stdout as a place to stash our original stderr. Thus, whether normal or vebose, when the management handoff is complete, copying stdout back to stderr does the right thing for the rest of the life of the daemon child. Note that while the error messages expected by iotest 233 are now restored, the change in file descriptors means they now show up earlier in the testsuite output. Reported-by: Kevin Wolf CC: Denis V. Lunev CC: qemu-sta...@nongnu.org Fixes: 5c56dd27a2 ("qemu-nbd: fix regression with qemu-nbd --fork run over ssh") Signed-off-by: Eric Blake --- qemu-nbd.c | 21 - tests/qemu-iotests/233.out | 20 ++-- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/qemu-nbd.c b/qemu-nbd.c index aaccaa33184..a105094fb17 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -944,9 +944,24 @@ int main(int argc, char **argv) close(stderr_fd[0]); -ret = qemu_daemon(1, 0); +ret = qemu_daemon(1, verbose); saved_errno = errno;/* dup2 will overwrite error below */ +if (verbose) { +/* We want stdin at /dev/null when qemu_daemon didn't do it */ +stdin = freopen("/dev/null", "r", stdin); +if (stdin == NULL) { +error_report("Failed to redirect stdin: %s", + strerror(errno)); +exit(EXIT_FAILURE); +} +/* To keep the parent's stderr alive, copy it to stdout */ +if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) { +error_report("Failed to redirect stdout: %s", + strerror(errno)); +exit(EXIT_FAILURE); +} +} /* Temporarily redirect stderr to the parent's pipe... */ if (dup2(stderr_fd[1], STDERR_FILENO) < 0) { char str[256]; @@ -1180,6 +1195,10 @@ int main(int argc, char **argv) } if (fork_process) { +/* + * See above. If verbose is false, stdout is /dev/null (thanks + * to qemu_daemon); otherwise, stdout is the parent's stderr. + */ if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) { error_report("Could not set stderr to /dev/null: %s", strerror(errno)); diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out index 237c82767ea..b09a197020a 100644 --- a/tests/qemu-iotests/233.out +++ b/tests/qemu-iotests/233.out @@ -41,8 +41,10 @@ exports available: 1 min block: 1 == check TLS fail over TCP with mismatched hostname == +qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort qemu-img: Could not open 'driver=nbd,host=localhost,port=PORT,tls-creds=tls0': Certificate does not match the hostname localhost qemu-nbd: Certificate does not match the hostname localhost +qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort == check TLS works over TCP with mismatched hostname and override == image: nbd://localhost:PORT @@ -55,7 +57,9 @@ exports available: 1 min block: 1 == check TLS with different CA fails == +qemu-nbd: option negotiation failed: Verify failed: No certificate was found. qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': The certificate hasn't got a known issuer +qemu-nbd: option negotiation failed: Verify failed: No certificate was found. qemu-nbd: The certificate hasn't got a known issuer == perform I/O over TLS == @@ -67,11 +71,15 @@ read 1048576/1048576 bytes at offset 1048576 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == check TLS with authorization == +qemu-nbd: option negotiation failed: TLS x509 authz check for C=South Pacific,L=R'lyeh,O=Cthulhu Dark Lord Enterprises client1,CN=localhost is denied qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort +qemu-nbd: option negotiation failed: TLS x509 authz check for C=South Pacific,L=R'lyeh,O=Cthulhu Dark Lord Enterprises client3,CN=localhost is denied qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort