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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.