| Message ID | 20230824200311.636589-2-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu-nbd: Restore "qemu-nbd -v --fork" output | expand |
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 <kwolf@redhat.com> > CC: Denis V. Lunev <den@openvz.org> > CC: qemu-stable@nongnu.org > Fixes: 5c56dd27a2 ("qemu-nbd: fix regression with qemu-nbd --fork run over ssh") > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > 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 > > == check TLS fail over UNIX with no 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,path=SOCK_DIR/qemu-nbd.sock,tls-creds=tls0': No hostname for certificate validation > +qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort > qemu-nbd: No hostname for certificate validation > > == check TLS works over UNIX with hostname override == > @@ -95,18 +103,10 @@ exports available: 1 > min block: 1 > > == check TLS fails over UNIX with mismatch PSK == > +qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received. > qemu-img: Could not open 'driver=nbd,path=SOCK_DIR/qemu-nbd.sock,tls-creds=tls0': TLS handshake failed: The TLS connection was non-properly terminated. > +qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received. > qemu-nbd: TLS handshake failed: The TLS connection was non-properly terminated. > > == final server log == > -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort > -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort > -qemu-nbd: option negotiation failed: Verify failed: No certificate was found. > -qemu-nbd: option negotiation failed: Verify failed: No certificate was found. > -qemu-nbd: option negotiation failed: TLS x509 authz check for DISTINGUISHED-NAME is denied > -qemu-nbd: option negotiation failed: TLS x509 authz check for DISTINGUISHED-NAME is denied > -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort > -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort > -qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received. > -qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received. > *** done May be alternative approach would be better. I have tried to send it as a reply here but failed. https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg04597.html Thank you in advance, Den
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 == check TLS fail over UNIX with no 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,path=SOCK_DIR/qemu-nbd.sock,tls-creds=tls0': No hostname for certificate validation +qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort qemu-nbd: No hostname for certificate validation == check TLS works over UNIX with hostname override == @@ -95,18 +103,10 @@ exports available: 1 min block: 1 == check TLS fails over UNIX with mismatch PSK == +qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received. qemu-img: Could not open 'driver=nbd,path=SOCK_DIR/qemu-nbd.sock,tls-creds=tls0': TLS handshake failed: The TLS connection was non-properly terminated. +qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received. qemu-nbd: TLS handshake failed: The TLS connection was non-properly terminated. == final server log == -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort -qemu-nbd: option negotiation failed: Verify failed: No certificate was found. -qemu-nbd: option negotiation failed: Verify failed: No certificate was found. -qemu-nbd: option negotiation failed: TLS x509 authz check for DISTINGUISHED-NAME is denied -qemu-nbd: option negotiation failed: TLS x509 authz check for DISTINGUISHED-NAME is denied -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort -qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received. -qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received. *** done
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 <kwolf@redhat.com> CC: Denis V. Lunev <den@openvz.org> CC: qemu-stable@nongnu.org Fixes: 5c56dd27a2 ("qemu-nbd: fix regression with qemu-nbd --fork run over ssh") Signed-off-by: Eric Blake <eblake@redhat.com> --- qemu-nbd.c | 21 ++++++++++++++++++++- tests/qemu-iotests/233.out | 20 ++++++++++---------- 2 files changed, 30 insertions(+), 11 deletions(-)