* [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
* 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
* [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: 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).