Skip to contentSkip to conversation

vl: Avoid to close stdout after finish 'writeconfig' option

PATCH In review Chen Gang 4 participants 7 messages Apr 20, 2014 – Apr 22, 2014

lore
Share & tools
lore

Chen Gang

author
#0 lore raw

PATCH 1/1 vl: Avoid to close stdout after finish 'writeconfig' option

After finish 'writeconfig' to stdout (with '-'), we want to copy/past the related information mannually, not for redirection ('readconfig' does not support '-').

So we can not close the stdout, or next options which may use stdout will not be displayed.

---
 vl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
vl.c +3 -1
index 9975e5a..215467f 100644
@@ -3855,7 +3855,9 @@ int main(int argc, char **argv, char **envp)
                         }
                     }
                     qemu_config_write(fp);
-                    fclose(fp);
+                    if (fp != stdout) {
+                        fclose(fp);
+                    }
                     break;
                 }
             case QEMU_OPTION_qtest:
Signature · 1 line
1.7.11.7
Reviewed-by on v1 1/1
#1 lore raw
Quoted 6 lines · Chen Gang

After finish 'writeconfig' to stdout (with '-'), we want to copy/past the related information mannually, not for redirection ('readconfig' does not support '-').

So we can not close the stdout, or next options which may use stdout will not be displayed.

Grammar, and awkward to read. May I suggest:

vl: avoid closing stdout with 'writeconfig'

'writeconfig' supports output to stdout (with '-'); when that happens, we must not close stdout, or further command line options that also use stdout will be impacted. (Although 'writeconfig' was copied from 'readconfig', the latter does not have the problem because it does not support reading from '-')

Quoted 6 lines
---
 vl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

With the improved commit message,

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
author
#2 lore raw
Quoted 20 lines · Eric Blake
Quoted 6 lines · Chen Gang

After finish 'writeconfig' to stdout (with '-'), we want to copy/past the related information mannually, not for redirection ('readconfig' does not support '-').

So we can not close the stdout, or next options which may use stdout will not be displayed.

Grammar, and awkward to read. May I suggest:

vl: avoid closing stdout with 'writeconfig'

'writeconfig' supports output to stdout (with '-'); when that happens, we must not close stdout, or further command line options that also use stdout will be impacted. (Although 'writeconfig' was copied from 'readconfig', the latter does not have the problem because it does not support reading from '-')

Quoted 2 lines
Quoted 10 lines
Quoted 4 lines
---
 vl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

With the improved commit message,

OK, it sounds fine to me, I shall send patch v2 for it.

Thanks.

Signature · 3 lines
Chen Gang

Open, share, and attitude like air, water, and life which God blessed
#3 lore raw
In reply to Eric Blake

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 21.04.2014 17:09, schrieb Eric Blake:

Quoted 10 lines · Chen Gang
Quoted 6 lines

After finish 'writeconfig' to stdout (with '-'), we want to copy/past the related information mannually, not for redirection ('readconfig' does not support '-').

So we can not close the stdout, or next options which may use stdout will not be displayed.

Grammar, and awkward to read. May I suggest:

Quoted 7 lines

vl: avoid closing stdout with 'writeconfig'

'writeconfig' supports output to stdout (with '-'); when that happens, we must not close stdout, or further command line options that also use stdout will be impacted. (Although 'writeconfig' was copied from 'readconfig', the latter does not have the problem because it does not support reading from '-')

Eric, did you intentionally request to change "Avoid" to lowercase? I was once pointed to https://wiki.gnome.org/Git/CommitMessages as a template, and starting an English-language heading with an uppercase letter seems natural, with anything else being too lazy to press Shift.

Personally I find -writeconfig the most straightforward way to indicate it's an option, just like () makes clear something is a function. The original subject had an explicit "option", that got lost for v2.

Regards,
Andreas

- --
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJTVpNxAAoJEPou0S0+fgE/NBEP/RbA40p75QIWuDBM31PXXZNo lrIxfLkb9QVvN7ALUsGOEn6Y1bNQWuItfq5YBN0oDDBxWKSHaU2ou8M3TUWIX6S8 ENLPFJzsVGbzsb4pkP0yMMWZdbs+8AChDJ+oZxgtz6eBf90ERYLarL/t8UjnZos4 wKU3alo2j9xf2OfMpqEHvtxcFc9URDZWKjElPLkZDPn5rFEwcaVGu78rDtjNQIU4 z7Ebc7li29Yppz9MrQ6QoXtU5ZGygPlzZ90G5UZc4v0H0u4ObyYInrWbxqryVp2H k+X173VVpJtPzlhzDkBJPdnwqhoExq7x/n2ujZicRyL95+8ZeoH2kteMRP0eHZYV TDLOkIq2FXklccn91FVVCk6GfLKLdcgZMotaxYD1EIxp7I9NlzpB0d1EVCgClWMx AgZ6StOhYt4+72BaLDOT602FHdsVSEnCqsQeBxoDhKWLwKLklKi59vzDGn+wH0Ap fY33zIW4+YAiXDv3vQg+1k8iG7BauO3cHDVQfbRMzU5zgWORMsm1lsnEv0b3x9YH PhuG9Ve9J2zrbBQEBe0zx98CyB0tp8L/8w5TUVD3EEmQA9YZP5y7I8WU8mOnvgRa Rpu3JF6U3KBO99VF0Yyd3K1mTcG4yJzGfxdLdiTIkSDbEV/jc5aoJFltldZWWpsl DigKmE0VxCFolXJsIsiX
=810z
-----END PGP SIGNATURE-----

#4 lore raw
Quoted 6 lines · Andreas Färber
Quoted 3 lines

Grammar, and awkward to read. May I suggest:

vl: avoid closing stdout with 'writeconfig'

Eric, did you intentionally request to change "Avoid" to lowercase?

Yes, out of habit, and in comparison to many recent commits. In fact, looking at the current qemu.git head 'git shortlog -100 2d03b49c' shows:

3 commits matching '\.$', 97 without
75 commits matching ': [A-Z]', 24 matching ': [a-z]' (some overlap here, and merge commits don't match either)

extending further to 1000 commits:

21 commits with trailing dot, 979 without
644 matching ': [A-Z]', 260 matching ': [a-z]'

at 10000, things are a bit more even:
5187 matching ': [A-Z]', 3904 matching ': [a-z]'

but still a distinct leaning towards capital at the start of the message

Quoted 3 lines

I was once pointed to https://wiki.gnome.org/Git/CommitMessages as a template, and starting an English-language heading with an uppercase letter seems natural, with anything else being too lazy to press Shift.

Hmm, maybe we should update the qemu wiki?
http://wiki.qemu.org/Contribute/SubmitAPatch doesn't mention the qemu preferred style (you pointed to the GNOME style, but at least GNU coreutils explicitly prefers lowercase).

At any rate, I'm not going to reject a patch based on capitalization, and can retrain my fingers to use a capital if that is the documented preference.

Quoted 4 lines

Personally I find -writeconfig the most straightforward way to indicate it's an option, just like () makes clear something is a function. The original subject had an explicit "option", that got lost for v2.

Good call, so maybe:

vl: Avoid closing stdout with '-writeconfig'

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#5 lore raw
Quoted 6 lines · Eric Blake

Hmm, maybe we should update the qemu wiki?
http://wiki.qemu.org/Contribute/SubmitAPatch doesn't mention the qemu preferred style (you pointed to the GNOME style, but at least GNU coreutils explicitly prefers lowercase).

At any rate, I'm not going to reject a patch based on capitalization,

Yeah, it doesn't really seem worth trying to enforce niceties of style here -- the effort required doesn't seem to really match the benefit, and we already have a lot of hoops we make new submitters jump through. "[file or area being patched]: [reasonably descriptive short summary]" is about the limit I personally feel comfortable insisting on.

thanks
-- PMM

#6 lore raw

[Qemu-trivial] commit log tips [was: [Qemu-devel] [PATCH trivial] vl: Avoid to close stdout after finish 'writeconfig' option]

Quoted 6 lines · Peter Maydell
Quoted 5 lines · Eric Blake

Hmm, maybe we should update the qemu wiki?
http://wiki.qemu.org/Contribute/SubmitAPatch doesn't mention the qemu preferred style (you pointed to the GNOME style, but at least GNU coreutils explicitly prefers lowercase).

Quoted 8 lines
Quoted 1 line

At any rate, I'm not going to reject a patch based on capitalization,

Yeah, it doesn't really seem worth trying to enforce niceties of style here -- the effort required doesn't seem to really match the benefit, and we already have a lot of hoops we make new submitters jump through. "[file or area being patched]: [reasonably descriptive short summary]" is about the limit I personally feel comfortable insisting on.

So, given this discussion, I enhanced the existing paragraph in the wiki page mentioned above to include a couple more sentences:

'''Write a good commit message'''. QEMU follows the usual standard for git commit messages: the first line (which becomes the email subject line) is "subsystem: single line summary of change". Whether the "single line summary of change" starts with a capital is a matter of taste, but we prefer that the summary does not end in ".". Look at <code>git short-log 30</code> for an idea of sample subject lines. Then there is a blank line and a more detailed description of the patch, another blank and your Signed-off-by: line. The body of the commit message is a good place to document why your change is important. Don't include comments like "This is a suggestion for fixing this bug" (they can go below the "---" line in the email so they don't go into the final commit message).

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org