[PATCH] qemu-nbd: Close inherited stderr

[PATCH] qemu-nbd: Close inherited stderr

[Raphael Pour]

[-- Attachment #1.1: Type: text/plain, Size: 1620 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] qemu-nbd: Close inherited stderr

[Eric Blake]

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

Re: [PATCH] qemu-nbd: Close inherited stderr

[Raphael Pour]

[-- Attachment #1.1: Type: text/plain, Size: 1131 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] qemu-nbd: Close inherited stderr

[Eric Blake]

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

Re: [PATCH] qemu-nbd: Close inherited stderr

[Raphael Pour]

[-- Attachment #1.1: Type: text/plain, Size: 1068 bytes --]



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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]