qemu-nbd.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
Hello,
after e6df58a5, the inherited stderr 'old_stderr' won't get closed
anymore if 'fork_process' is false. This causes other processes relying
on EOF to infinitely block or crash.
From 47ab9b517038d13117876a8bb3ef45c53d7f2f9e Mon Sep 17 00:00:00 2001
From: "Raphael Pour" <raphael.pour@hetzner.com>
Date: Tue, 12 May 2020 10:18:44 +0200
Subject: [PATCH] qemu-nbd: Close inherited stderr
Close inherited stderr of the parent if fork_process is false.
Otherwise no one will close it. (introduced by e6df58a5)
Signed-off-by: Raphael Pour <raphael.pour@hetzner.com>
---
qemu-nbd.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 108a51f7e..f2981e18a 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1032,8 +1032,15 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}
- /* ... close the descriptor we inherited and go on. */
+ /* ... close the descriptor we inherited and ... */
close(stderr_fd[1]);
+
+ /* ... also close the old_stderr IF fork_process is false
otherwise
+ * it will never get closed.
+ */
+ if (!fork_process) {
+ close(old_stderr);
+ }
} else {
bool errors = false;
char *buf;
--
2.25.4
--
Hetzner Online GmbH
Am Datacenter-Park 1
08223 Falkenstein/Vogtland
raphael.pour@hetzner.com
www.hetzner.com
Registergericht Ansbach, HRB 6089
Geschäftsführer: Martin Hetzner, Stephan Konvickova, Günther Müller
On 5/12/20 3:57 PM, Eric Blake wrote:
> Wouldn't it just be simpler to not dup in the first place?
>
> diff --git i/qemu-nbd.c w/qemu-nbd.c
> index 4aa005004ebd..6ba2544feb3a 100644
> --- i/qemu-nbd.c
> +++ w/qemu-nbd.c
> @@ -916,7 +916,9 @@ int main(int argc, char **argv)
> } else if (pid == 0) {
> close(stderr_fd[0]);
>
> - old_stderr = dup(STDERR_FILENO);
> + if (fork_process) {
> + old_stderr = dup(STDERR_FILENO);
> + }
> ret = qemu_daemon(1, 0);
>
> /* Temporarily redirect stderr to the parent's pipe... */
Yes, you're right. We tested your patch and it also fixes the unwanted
open stderr.
Could you consider this patch in one of the next releases?
Thanks!
Raphael
--
Hetzner Online GmbH
Am Datacenter-Park 1
08223 Falkenstein/Vogtland
raphael.pour@hetzner.com
www.hetzner.com
Registergericht Ansbach, HRB 6089
Geschäftsführer: Martin Hetzner, Stephan Konvickova, Günther Müller
On 5/13/20 3:02 PM, Eric Blake wrote: > Yes, now that we know about it, the bug will be fixed in 5.1; we can > also cc: qemu-stable to get it backported to the next 5.0.x release > (downstream developers are also more likely to backport it to their > ports as well if it lands on qemu-stable). Would you like to try your > hand at a v2 patch, or shall I just turn my proposal into a formal patch > and mention you as the reporter? Time-wise, it may be faster for me to > just take over the patch than to spend the time coaching you through to > the point of your first successful submission, but project-wise, it is > always better to welcome in new contributors and share the wealth, > rather than making maintainers become the bottleneck. > I'll give v2 a try and cc qemu-stable. Thanks for the opportunity. -- Hetzner Online GmbH Am Datacenter-Park 1 08223 Falkenstein/Vogtland raphael.pour@hetzner.com www.hetzner.com Registergericht Ansbach, HRB 6089 Geschäftsführer: Martin Hetzner, Stephan Konvickova, Günther Müller
On 5/13/20 2:14 AM, Raphael Pour wrote:
> On 5/12/20 3:57 PM, Eric Blake wrote:
>> Wouldn't it just be simpler to not dup in the first place?
>>
>> diff --git i/qemu-nbd.c w/qemu-nbd.c
>> index 4aa005004ebd..6ba2544feb3a 100644
>> --- i/qemu-nbd.c
>> +++ w/qemu-nbd.c
>> @@ -916,7 +916,9 @@ int main(int argc, char **argv)
>> } else if (pid == 0) {
>> close(stderr_fd[0]);
>>
>> - old_stderr = dup(STDERR_FILENO);
>> + if (fork_process) {
>> + old_stderr = dup(STDERR_FILENO);
>> + }
>> ret = qemu_daemon(1, 0);
>>
>> /* Temporarily redirect stderr to the parent's pipe... */
>
> Yes, you're right. We tested your patch and it also fixes the unwanted
> open stderr.
>
> Could you consider this patch in one of the next releases?
Yes, now that we know about it, the bug will be fixed in 5.1; we can
also cc: qemu-stable to get it backported to the next 5.0.x release
(downstream developers are also more likely to backport it to their
ports as well if it lands on qemu-stable). Would you like to try your
hand at a v2 patch, or shall I just turn my proposal into a formal patch
and mention you as the reporter? Time-wise, it may be faster for me to
just take over the patch than to spend the time coaching you through to
the point of your first successful submission, but project-wise, it is
always better to welcome in new contributors and share the wealth,
rather than making maintainers become the bottleneck.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 5/12/20 3:56 AM, Raphael Pour wrote:
> Hello,
>
> after e6df58a5, the inherited stderr 'old_stderr' won't get closed
> anymore if 'fork_process' is false. This causes other processes relying
> on EOF to infinitely block or crash.
>
>>From 47ab9b517038d13117876a8bb3ef45c53d7f2f9e Mon Sep 17 00:00:00 2001
> From: "Raphael Pour" <raphael.pour@hetzner.com>
> Date: Tue, 12 May 2020 10:18:44 +0200
> Subject: [PATCH] qemu-nbd: Close inherited stderr
>
> Close inherited stderr of the parent if fork_process is false.
> Otherwise no one will close it. (introduced by e6df58a5)
>
> Signed-off-by: Raphael Pour <raphael.pour@hetzner.com>
> ---
> qemu-nbd.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
Wouldn't it just be simpler to not dup in the first place?
diff --git i/qemu-nbd.c w/qemu-nbd.c
index 4aa005004ebd..6ba2544feb3a 100644
--- i/qemu-nbd.c
+++ w/qemu-nbd.c
@@ -916,7 +916,9 @@ int main(int argc, char **argv)
} else if (pid == 0) {
close(stderr_fd[0]);
- old_stderr = dup(STDERR_FILENO);
+ if (fork_process) {
+ old_stderr = dup(STDERR_FILENO);
+ }
ret = qemu_daemon(1, 0);
/* Temporarily redirect stderr to the parent's pipe... */
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
© 2016 - 2025 Red Hat, Inc.