[PATCH v2 0/1] qemu-nbd: Close inherited stderr

[PATCH v2 0/1] qemu-nbd: Close inherited stderr

[Raphael Pour]

Hello,

introduced with e6df58a5, stderr won't get closed if the fork option is
set. This causes other processes reading stderr to block infinietly or
crash while relying on EOF.

v2:
  - Instead of closing the inherited stderr in the child, avoid the dup 
    in the parent if the fork option is not set.

Raphael Pour (1):
  qemu-nbd: Close inherited stderr

 qemu-nbd.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.25.4

[PATCH v2 1/1] qemu-nbd: Close inherited stderr

[Raphael Pour]

Close inherited stderr of the parent if fork_process is false.
Otherwise no one will close it. (introduced by e6df58a5)
---
 qemu-nbd.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 4aa005004e..a324d21c5e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -916,7 +916,13 @@ int main(int argc, char **argv)
         } else if (pid == 0) {
             close(stderr_fd[0]);
 
-            old_stderr = dup(STDERR_FILENO);
+            /* Remember parents stderr only if the fork option is set.
+             * The child won't close this inherited fd otherwise.
+             */
+            if (fork_process) {
+              old_stderr = dup(STDERR_FILENO);
+            }
+
             ret = qemu_daemon(1, 0);
 
             /* Temporarily redirect stderr to the parent's pipe...  */
-- 
2.25.4

Re: [PATCH v2 0/1] qemu-nbd: Close inherited stderr

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200514063112.1457125-1-raphael.pour@hetzner.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200514063112.1457125-1-raphael.pour@hetzner.com
Subject: [PATCH v2 0/1] qemu-nbd: Close inherited stderr
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200514122334.25089-1-kraxel@redhat.com -> patchew/20200514122334.25089-1-kraxel@redhat.com
 * [new tag]         patchew/20200514123729.156283-1-frankja@linux.ibm.com -> patchew/20200514123729.156283-1-frankja@linux.ibm.com
Switched to a new branch 'test'
76bb928 qemu-nbd: Close inherited stderr

=== OUTPUT BEGIN ===
WARNING: Block comments use a leading /* on a separate line
#20: FILE: qemu-nbd.c:919:
+            /* Remember parents stderr only if the fork option is set.

ERROR: suspect code indent for conditional statements (12, 14)
#23: FILE: qemu-nbd.c:922:
+            if (fork_process) {
+              old_stderr = dup(STDERR_FILENO);

ERROR: Missing Signed-off-by: line(s)

total: 2 errors, 1 warnings, 14 lines checked

Commit 76bb928d8364 (qemu-nbd: Close inherited stderr) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200514063112.1457125-1-raphael.pour@hetzner.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 0/1] qemu-nbd: Close inherited stderr

[Eric Blake]

On 5/14/20 7:51 AM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200514063112.1457125-1-raphael.pour@hetzner.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Message-id: 20200514063112.1457125-1-raphael.pour@hetzner.com
> Subject: [PATCH v2 0/1] qemu-nbd: Close inherited stderr
> Type: series
> 

> 
> === OUTPUT BEGIN ===
> WARNING: Block comments use a leading /* on a separate line
> #20: FILE: qemu-nbd.c:919:
> +            /* Remember parents stderr only if the fork option is set.
> 
> ERROR: suspect code indent for conditional statements (12, 14)
> #23: FILE: qemu-nbd.c:922:
> +            if (fork_process) {
> +              old_stderr = dup(STDERR_FILENO);
> 
> ERROR: Missing Signed-off-by: line(s)

Patchew is correct.  All three things should be fixed (the missing S-o-b 
is most important - I can't supply that; the missing spaces and comment 
formatting are something I could touch up).

The comment could use some grammar help (s/parents/parent's/), and in 
truth, I don't think it adds much beyond what the code itself is already 
doing, so rather than adding another line to silence patchew, you could 
instead just eliminate the comment and life would still be fine.  Or if 
you want a one-line comment, I might suggest:

/* Remember parent's stderr if we will restoring it. */

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 0/1] qemu-nbd: Close inherited stderr

[Eric Blake]

On 5/14/20 9:29 AM, Eric Blake wrote:

>> WARNING: Block comments use a leading /* on a separate line
>> #20: FILE: qemu-nbd.c:919:
>> +            /* Remember parents stderr only if the fork option is set.
>>

> The comment could use some grammar help (s/parents/parent's/), and in 
> truth, I don't think it adds much beyond what the code itself is already 
> doing, so rather than adding another line to silence patchew, you could 
> instead just eliminate the comment and life would still be fine.  Or if 
> you want a one-line comment, I might suggest:
> 
> /* Remember parent's stderr if we will restoring it. */

It helps if I don't hit 'send' too early.

/* Remember parent's stderr if we will be restoring it. */

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 0/1] qemu-nbd: Close inherited stderr

[Raphael Pour]

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

On 5/14/20 4:55 PM, Eric Blake wrote:
> On 5/14/20 9:29 AM, Eric Blake wrote:
> 
>>> WARNING: Block comments use a leading /* on a separate line
>>> #20: FILE: qemu-nbd.c:919:
>>> +            /* Remember parents stderr only if the fork option is set.
>>>
> 
>> The comment could use some grammar help (s/parents/parent's/), and in
>> truth, I don't think it adds much beyond what the code itself is
>> already doing, so rather than adding another line to silence patchew,
>> you could instead just eliminate the comment and life would still be
>> fine.  Or if you want a one-line comment, I might suggest:
>>
>> /* Remember parent's stderr if we will restoring it. */
> 
> It helps if I don't hit 'send' too early.
> 
> /* Remember parent's stderr if we will be restoring it. */
> 

From e5749541494abcdcaa37d752172741e1bc38e984 Mon Sep 17 00:00:00 2001
From: Raphael Pour <raphael.pour@hetzner.com>
Date: Fri, 15 May 2020 08:30:50 +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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 4aa005004e..306e44fb0a 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -916,7 +916,11 @@ int main(int argc, char **argv)
         } else if (pid == 0) {
             close(stderr_fd[0]);

-            old_stderr = dup(STDERR_FILENO);
+            /* Remember parent's stderr if we will be restoring it. */
+            if (fork_process) {
+                old_stderr = dup(STDERR_FILENO);
+            }
+
             ret = qemu_daemon(1, 0);

             /* Temporarily redirect stderr to the parent's pipe...  */
-- 
2.25.4


I fixed the issues and checkpatch gave me a 'ready for submission'.
Is this the right way to submit a fixed patch or should it be a v3?


-- 
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 v2 0/1] qemu-nbd: Close inherited stderr

[Eric Blake]

On 5/15/20 1:36 AM, Raphael Pour wrote:
>>From e5749541494abcdcaa37d752172741e1bc38e984 Mon Sep 17 00:00:00 2001
> From: Raphael Pour <raphael.pour@hetzner.com>
> Date: Fri, 15 May 2020 08:30:50 +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 | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)

Thanks. It might have been easier posting this as a standalone v3 rather 
than buried in reply to v2, but I have now queued it to go in through my 
NBD tree (I plan on doing a pull request Monday).

I've also taken the liberty to amend the commit message as follows, to 
give more context into why the fix is needed:

     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)

     This only affected 'qemu-nbd -c /dev/nbd0'.

     Signed-off-by: Raphael Pour <raphael.pour@hetzner.com>
     Message-Id: <d8ddc993-9816-836e-a3de-c6edab9d9c49@hetzner.com>
     Reviewed-by: Eric Blake <eblake@redhat.com>
     [eblake: Enhance commit message]
     Signed-off-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 0/1] qemu-nbd: Close inherited stderr

[Raphael Pour]

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

[...] introduced with e6df58a5, stderr won't get closed if the fork
option is __not__ set.

On 5/14/20 8:31 AM, Raphael Pour wrote:
> introduced with e6df58a5, stderr won't get closed if the fork option is
> set.

-- 
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 --]