* [PATCH] kvm: close opened file
@ 2013-01-16 1:07 Cong Ding
2013-01-16 1:07 ` [PATCH] kvm: remove redundant "if" condition Cong Ding
2013-01-16 11:35 ` [PATCH] kvm: close opened file Gleb Natapov
0 siblings, 2 replies; 9+ messages in thread
From: Cong Ding @ 2013-01-16 1:07 UTC (permalink / raw)
To: Pekka Enberg, Sasha Levin, Asias He, kvm, linux-kernel; +Cc: Cong Ding
The file should be closed before return.
Signed-off-by: Cong Ding <dinggnu@gmail.com>
---
tools/kvm/builtin-setup.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/kvm/builtin-setup.c b/tools/kvm/builtin-setup.c
index c5b0566..8b45c56 100644
--- a/tools/kvm/builtin-setup.c
+++ b/tools/kvm/builtin-setup.c
@@ -159,12 +159,12 @@ static int copy_passwd(const char *guestfs_name)
return -1;
ret = fprintf(file, "root:x:0:0:root:/root:/bin/sh\n");
- if (ret < 0)
- return ret;
+ if (ret > 0)
+ ret = 0;
fclose(file);
- return 0;
+ return ret;
}
static int make_guestfs_symlink(const char *guestfs_name, const char *path)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] kvm: remove redundant "if" condition
2013-01-16 1:07 [PATCH] kvm: close opened file Cong Ding
@ 2013-01-16 1:07 ` Cong Ding
2013-01-16 11:35 ` [PATCH] kvm: close opened file Gleb Natapov
1 sibling, 0 replies; 9+ messages in thread
From: Cong Ding @ 2013-01-16 1:07 UTC (permalink / raw)
To: Pekka Enberg, Sasha Levin, Asias He, kvm, linux-kernel; +Cc: Cong Ding
After we check (state.kcount != 0), state.kcount has to be 0 in all the "else"
branchs.
Signed-off-by: Cong Ding <dinggnu@gmail.com>
---
tools/kvm/hw/i8042.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/kvm/hw/i8042.c b/tools/kvm/hw/i8042.c
index 9f8be6a..9035732 100644
--- a/tools/kvm/hw/i8042.c
+++ b/tools/kvm/hw/i8042.c
@@ -189,7 +189,7 @@ static u32 kbd_read_data(void)
state.mcount--;
kvm__irq_line(state.kvm, AUX_IRQ, 0);
kbd_update_irq();
- } else if (state.kcount == 0) {
+ } else {
i = state.kread - 1;
if (i < 0)
i = QUEUE_SIZE;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] kvm: close opened file
2013-01-16 1:07 [PATCH] kvm: close opened file Cong Ding
2013-01-16 1:07 ` [PATCH] kvm: remove redundant "if" condition Cong Ding
@ 2013-01-16 11:35 ` Gleb Natapov
2013-01-16 16:51 ` [PATCH v2] kvm tools: " Cong Ding
2013-01-16 16:52 ` [PATCH v2] kvm tools: remove redundant "if" condition Cong Ding
1 sibling, 2 replies; 9+ messages in thread
From: Gleb Natapov @ 2013-01-16 11:35 UTC (permalink / raw)
To: Cong Ding; +Cc: Pekka Enberg, Sasha Levin, Asias He, kvm, linux-kernel
Please use "kvmtool:" prefix at the subject line.
On Wed, Jan 16, 2013 at 02:07:35AM +0100, Cong Ding wrote:
> The file should be closed before return.
>
> Signed-off-by: Cong Ding <dinggnu@gmail.com>
> ---
> tools/kvm/builtin-setup.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/kvm/builtin-setup.c b/tools/kvm/builtin-setup.c
> index c5b0566..8b45c56 100644
> --- a/tools/kvm/builtin-setup.c
> +++ b/tools/kvm/builtin-setup.c
> @@ -159,12 +159,12 @@ static int copy_passwd(const char *guestfs_name)
> return -1;
>
> ret = fprintf(file, "root:x:0:0:root:/root:/bin/sh\n");
> - if (ret < 0)
> - return ret;
> + if (ret > 0)
> + ret = 0;
>
> fclose(file);
>
> - return 0;
> + return ret;
> }
>
> static int make_guestfs_symlink(const char *guestfs_name, const char *path)
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Gleb.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] kvm tools: close opened file
2013-01-16 11:35 ` [PATCH] kvm: close opened file Gleb Natapov
@ 2013-01-16 16:51 ` Cong Ding
2013-01-19 8:57 ` Pekka Enberg
2013-01-16 16:52 ` [PATCH v2] kvm tools: remove redundant "if" condition Cong Ding
1 sibling, 1 reply; 9+ messages in thread
From: Cong Ding @ 2013-01-16 16:51 UTC (permalink / raw)
To: Gleb Natapov; +Cc: Pekka Enberg, Sasha Levin, Asias He, kvm, linux-kernel
The file should be closed before return.
Signed-off-by: Cong Ding <dinggnu@gmail.com>
---
tools/kvm/builtin-setup.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/kvm/builtin-setup.c b/tools/kvm/builtin-setup.c
index c5b0566..8b45c56 100644
--- a/tools/kvm/builtin-setup.c
+++ b/tools/kvm/builtin-setup.c
@@ -159,12 +159,12 @@ static int copy_passwd(const char *guestfs_name)
return -1;
ret = fprintf(file, "root:x:0:0:root:/root:/bin/sh\n");
- if (ret < 0)
- return ret;
+ if (ret > 0)
+ ret = 0;
fclose(file);
- return 0;
+ return ret;
}
static int make_guestfs_symlink(const char *guestfs_name, const char *path)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2] kvm tools: remove redundant "if" condition
2013-01-16 11:35 ` [PATCH] kvm: close opened file Gleb Natapov
2013-01-16 16:51 ` [PATCH v2] kvm tools: " Cong Ding
@ 2013-01-16 16:52 ` Cong Ding
2013-01-19 8:58 ` Pekka Enberg
1 sibling, 1 reply; 9+ messages in thread
From: Cong Ding @ 2013-01-16 16:52 UTC (permalink / raw)
To: Gleb Natapov; +Cc: Pekka Enberg, Sasha Levin, Asias He, kvm, linux-kernel
After we check (state.kcount != 0), state.kcount has to be 0 in all the "else"
branchs.
Signed-off-by: Cong Ding <dinggnu@gmail.com>
---
tools/kvm/hw/i8042.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/kvm/hw/i8042.c b/tools/kvm/hw/i8042.c
index 9f8be6a..9035732 100644
--- a/tools/kvm/hw/i8042.c
+++ b/tools/kvm/hw/i8042.c
@@ -189,7 +189,7 @@ static u32 kbd_read_data(void)
state.mcount--;
kvm__irq_line(state.kvm, AUX_IRQ, 0);
kbd_update_irq();
- } else if (state.kcount == 0) {
+ } else {
i = state.kread - 1;
if (i < 0)
i = QUEUE_SIZE;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kvm tools: close opened file
2013-01-16 16:51 ` [PATCH v2] kvm tools: " Cong Ding
@ 2013-01-19 8:57 ` Pekka Enberg
0 siblings, 0 replies; 9+ messages in thread
From: Pekka Enberg @ 2013-01-19 8:57 UTC (permalink / raw)
To: Cong Ding; +Cc: Gleb Natapov, Sasha Levin, Asias He, kvm, linux-kernel
On Wed, Jan 16, 2013 at 6:51 PM, Cong Ding <dinggnu@gmail.com> wrote:
> The file should be closed before return.
>
> Signed-off-by: Cong Ding <dinggnu@gmail.com>
Applied, thanks!
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kvm tools: remove redundant "if" condition
2013-01-16 16:52 ` [PATCH v2] kvm tools: remove redundant "if" condition Cong Ding
@ 2013-01-19 8:58 ` Pekka Enberg
2013-01-19 10:27 ` Cong Ding
0 siblings, 1 reply; 9+ messages in thread
From: Pekka Enberg @ 2013-01-19 8:58 UTC (permalink / raw)
To: Cong Ding; +Cc: Gleb Natapov, Sasha Levin, Asias He, kvm, linux-kernel
On Wed, Jan 16, 2013 at 6:52 PM, Cong Ding <dinggnu@gmail.com> wrote:
> After we check (state.kcount != 0), state.kcount has to be 0 in all the "else"
> branchs.
>
> Signed-off-by: Cong Ding <dinggnu@gmail.com>
> ---
> tools/kvm/hw/i8042.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/kvm/hw/i8042.c b/tools/kvm/hw/i8042.c
> index 9f8be6a..9035732 100644
> --- a/tools/kvm/hw/i8042.c
> +++ b/tools/kvm/hw/i8042.c
> @@ -189,7 +189,7 @@ static u32 kbd_read_data(void)
> state.mcount--;
> kvm__irq_line(state.kvm, AUX_IRQ, 0);
> kbd_update_irq();
> - } else if (state.kcount == 0) {
> + } else {
> i = state.kread - 1;
> if (i < 0)
> i = QUEUE_SIZE;
This doesn't look right. The 'kcount' field is an int so the value can
be negative.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kvm tools: remove redundant "if" condition
2013-01-19 8:58 ` Pekka Enberg
@ 2013-01-19 10:27 ` Cong Ding
2013-01-22 19:52 ` Pekka Enberg
0 siblings, 1 reply; 9+ messages in thread
From: Cong Ding @ 2013-01-19 10:27 UTC (permalink / raw)
To: Pekka Enberg; +Cc: Gleb Natapov, Sasha Levin, Asias He, kvm, linux-kernel
On Sat, Jan 19, 2013 at 10:58:33AM +0200, Pekka Enberg wrote:
> On Wed, Jan 16, 2013 at 6:52 PM, Cong Ding <dinggnu@gmail.com> wrote:
> > After we check (state.kcount != 0), state.kcount has to be 0 in all the "else"
> > branchs.
> >
> > Signed-off-by: Cong Ding <dinggnu@gmail.com>
> > ---
> > tools/kvm/hw/i8042.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/kvm/hw/i8042.c b/tools/kvm/hw/i8042.c
> > index 9f8be6a..9035732 100644
> > --- a/tools/kvm/hw/i8042.c
> > +++ b/tools/kvm/hw/i8042.c
> > @@ -189,7 +189,7 @@ static u32 kbd_read_data(void)
> > state.mcount--;
> > kvm__irq_line(state.kvm, AUX_IRQ, 0);
> > kbd_update_irq();
> > - } else if (state.kcount == 0) {
> > + } else {
> > i = state.kread - 1;
> > if (i < 0)
> > i = QUEUE_SIZE;
>
> This doesn't look right. The 'kcount' field is an int so the value can
> be negative.
But the former check is "state.kcount != 0" as I described in the commit
message. Notice the difference between variable names in the "if" condition: the
first one is kcount, the second one is mcount, and the third one is same as the
first one kcount.
Ok, the original code is
if (state.kcount != 0) {
/* do something when (state.kcount != 0) */
} else if (state.mcount > 0) {
/* do something when (state.kcount == 0 && state.mount > 0) */
} else if (state.kcount == 0) {
/* do something when (state.kcount == 0 && state.mount <= 0) */
}
For the third branch, it runs when state.kcount == 0 and state.mount <= 0,
it's not necessary to ensure state.kcount == 0 again.
- cong
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kvm tools: remove redundant "if" condition
2013-01-19 10:27 ` Cong Ding
@ 2013-01-22 19:52 ` Pekka Enberg
0 siblings, 0 replies; 9+ messages in thread
From: Pekka Enberg @ 2013-01-22 19:52 UTC (permalink / raw)
To: Cong Ding; +Cc: Gleb Natapov, Sasha Levin, Asias He, kvm, linux-kernel
On Sat, Jan 19, 2013 at 12:27 PM, Cong Ding <dinggnu@gmail.com> wrote:
> On Sat, Jan 19, 2013 at 10:58:33AM +0200, Pekka Enberg wrote:
>> On Wed, Jan 16, 2013 at 6:52 PM, Cong Ding <dinggnu@gmail.com> wrote:
>> > After we check (state.kcount != 0), state.kcount has to be 0 in all the "else"
>> > branchs.
>> >
>> > Signed-off-by: Cong Ding <dinggnu@gmail.com>
>> > ---
>> > tools/kvm/hw/i8042.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/tools/kvm/hw/i8042.c b/tools/kvm/hw/i8042.c
>> > index 9f8be6a..9035732 100644
>> > --- a/tools/kvm/hw/i8042.c
>> > +++ b/tools/kvm/hw/i8042.c
>> > @@ -189,7 +189,7 @@ static u32 kbd_read_data(void)
>> > state.mcount--;
>> > kvm__irq_line(state.kvm, AUX_IRQ, 0);
>> > kbd_update_irq();
>> > - } else if (state.kcount == 0) {
>> > + } else {
>> > i = state.kread - 1;
>> > if (i < 0)
>> > i = QUEUE_SIZE;
>>
>> This doesn't look right. The 'kcount' field is an int so the value can
>> be negative.
> But the former check is "state.kcount != 0" as I described in the commit
> message. Notice the difference between variable names in the "if" condition: the
> first one is kcount, the second one is mcount, and the third one is same as the
> first one kcount.
>
> Ok, the original code is
> if (state.kcount != 0) {
> /* do something when (state.kcount != 0) */
> } else if (state.mcount > 0) {
> /* do something when (state.kcount == 0 && state.mount > 0) */
> } else if (state.kcount == 0) {
> /* do something when (state.kcount == 0 && state.mount <= 0) */
> }
> For the third branch, it runs when state.kcount == 0 and state.mount <= 0,
> it's not necessary to ensure state.kcount == 0 again.
Right you are. Applied, thanks!
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-01-22 19:53 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-16 1:07 [PATCH] kvm: close opened file Cong Ding
2013-01-16 1:07 ` [PATCH] kvm: remove redundant "if" condition Cong Ding
2013-01-16 11:35 ` [PATCH] kvm: close opened file Gleb Natapov
2013-01-16 16:51 ` [PATCH v2] kvm tools: " Cong Ding
2013-01-19 8:57 ` Pekka Enberg
2013-01-16 16:52 ` [PATCH v2] kvm tools: remove redundant "if" condition Cong Ding
2013-01-19 8:58 ` Pekka Enberg
2013-01-19 10:27 ` Cong Ding
2013-01-22 19:52 ` Pekka Enberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).