[Qemu-trivial] [PATCH trivial] vl: Avoid to close stdout after finish 'writeconfig' option

[Qemu-trivial] [PATCH trivial] vl: Avoid to close stdout after finish 'writeconfig' option

[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.


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 vl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 9975e5a..215467f 100644
--- a/vl.c
+++ b/vl.c
@@ -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:
-- 
1.7.11.7

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial] vl: Avoid to close stdout after finish 'writeconfig' option

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

On 04/20/2014 06:05 AM, Chen Gang wrote:
> 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 '-')

> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  vl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

With the improved commit message,

Reviewed-by: Eric Blake <eblake@redhat.com>


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


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

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial] vl: Avoid to close stdout after finish 'writeconfig' option

[Chen Gang]

On 04/21/2014 11:09 PM, Eric Blake wrote:
> On 04/20/2014 06:05 AM, Chen Gang wrote:
>> 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 '-')
> 
>>
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  vl.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> With the improved commit message,
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> 

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

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial] vl: Avoid to close stdout after finish 'writeconfig' option

[Andreas Färber]

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

Am 21.04.2014 17:09, schrieb Eric Blake:
> On 04/20/2014 06:05 AM, Chen Gang wrote:
>> 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 '-')

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial] vl: Avoid to close stdout after finish 'writeconfig' option

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]

On 04/22/2014 10:06 AM, Andreas Färber wrote:

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

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

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

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


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

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial] vl: Avoid to close stdout after finish 'writeconfig' option

[Peter Maydell]

On 22 April 2014 17:31, Eric Blake <eblake@redhat.com> wrote:
> 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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1834 bytes --]

On 04/22/2014 01:18 PM, Peter Maydell wrote:
> On 22 April 2014 17:31, Eric Blake <eblake@redhat.com> wrote:
>> 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.

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).

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


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