* [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue @ 2015-06-30 21:35 Sohny Thomas 2015-07-01 6:57 ` Sudip Mukherjee 0 siblings, 1 reply; 9+ messages in thread From: Sohny Thomas @ 2015-06-30 21:35 UTC (permalink / raw) To: benjamin.romer, david.kershner, bryan.thompson, erik.arfvidson, devel Cc: gregkh, sparmaintainer, linux-kernel, kernel-janitors FIX 2 unnecessary braces found by checkpatch.pl Signed-off-by: Sohny Thomas <sohnythomas@zoho.com> --- drivers/staging/unisys/virtpci/virtpci.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/staging/unisys/virtpci/virtpci.c b/drivers/staging/unisys/virtpci/virtpci.c index d5ad017..f3674de 100644 --- a/drivers/staging/unisys/virtpci/virtpci.c +++ b/drivers/staging/unisys/virtpci/virtpci.c @@ -190,9 +190,10 @@ static int write_vbus_chp_info(struct spar_vbus_channel_protocol *chan, return -1; off = sizeof(struct channel_header) + chan->hdr_info.chp_info_offset; - if (chan->hdr_info.chp_info_offset == 0) { + + if (chan->hdr_info.chp_info_offset == 0) return -1; - } + memcpy(((u8 *)(chan)) + off, info, sizeof(*info)); return 0; } @@ -484,10 +485,10 @@ static int delete_vhba(struct del_virt_guestpart *delparams) i = virtpci_device_del(NULL /*no parent bus */, VIRTHBA_TYPE, &scsi.wwnn, NULL); - if (i) { + if (i) return 1; - } - return 0; + else + return 0; } /* deletes a vnic -- ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue 2015-06-30 21:35 [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue Sohny Thomas @ 2015-07-01 6:57 ` Sudip Mukherjee 2015-07-01 7:36 ` Sohny Thomas 0 siblings, 1 reply; 9+ messages in thread From: Sudip Mukherjee @ 2015-07-01 6:57 UTC (permalink / raw) To: Sohny Thomas Cc: benjamin.romer, david.kershner, bryan.thompson, erik.arfvidson, devel, gregkh, sparmaintainer, kernel-janitors, linux-kernel On Wed, Jul 01, 2015 at 03:05:45AM +0530, Sohny Thomas wrote: > > FIX 2 unnecessary braces found by checkpatch.pl > > Signed-off-by: Sohny Thomas <sohnythomas@zoho.com> > --- > drivers/staging/unisys/virtpci/virtpci.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/staging/unisys/virtpci/virtpci.c b/drivers/staging/unisys/virtpci/virtpci.c > index d5ad017..f3674de 100644 > --- a/drivers/staging/unisys/virtpci/virtpci.c > +++ b/drivers/staging/unisys/virtpci/virtpci.c > @@ -190,9 +190,10 @@ static int write_vbus_chp_info(struct spar_vbus_channel_protocol *chan, > return -1; > > off = sizeof(struct channel_header) + chan->hdr_info.chp_info_offset; > - if (chan->hdr_info.chp_info_offset == 0) { > + > + if (chan->hdr_info.chp_info_offset == 0) > return -1; > - } > + why you are inserting new line here? > memcpy(((u8 *)(chan)) + off, info, sizeof(*info)); > return 0; > } > @@ -484,10 +485,10 @@ static int delete_vhba(struct del_virt_guestpart *delparams) > > i = virtpci_device_del(NULL /*no parent bus */, VIRTHBA_TYPE, > &scsi.wwnn, NULL); > - if (i) { > + if (i) > return 1; > - } > - return 0; > + else > + return 0; No, now this will introduce a new checkpatch warning that "else is not required after return". why did you introduce this "else"? regards sudip ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue 2015-07-01 6:57 ` Sudip Mukherjee @ 2015-07-01 7:36 ` Sohny Thomas 2015-07-01 8:01 ` Julia Lawall 2015-07-01 8:02 ` Sudip Mukherjee 0 siblings, 2 replies; 9+ messages in thread From: Sohny Thomas @ 2015-07-01 7:36 UTC (permalink / raw) To: Sudip Mukherjee Cc: benjamin.romer, david.kershner, bryan.thompson, erik.arfvidson, devel, gregkh, sparmaintainer, kernel-janitors, linux-kernel Thanks for review, my answers inline On 01-07-2015 12:27, Sudip Mukherjee wrote: > On Wed, Jul 01, 2015 at 03:05:45AM +0530, Sohny Thomas wrote: >> >> FIX 2 unnecessary braces found by checkpatch.pl >> >> Signed-off-by: Sohny Thomas <sohnythomas@zoho.com> >> --- >> drivers/staging/unisys/virtpci/virtpci.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/staging/unisys/virtpci/virtpci.c b/drivers/staging/unisys/virtpci/virtpci.c >> index d5ad017..f3674de 100644 >> --- a/drivers/staging/unisys/virtpci/virtpci.c >> +++ b/drivers/staging/unisys/virtpci/virtpci.c >> @@ -190,9 +190,10 @@ static int write_vbus_chp_info(struct spar_vbus_channel_protocol *chan, >> return -1; >> >> off = sizeof(struct channel_header) + chan->hdr_info.chp_info_offset; >> - if (chan->hdr_info.chp_info_offset == 0) { >> + >> + if (chan->hdr_info.chp_info_offset == 0) >> return -1; >> - } >> + > why you are inserting new line here? I did it so that its readable, will remove it if not required > >> memcpy(((u8 *)(chan)) + off, info, sizeof(*info)); >> return 0; >> } >> @@ -484,10 +485,10 @@ static int delete_vhba(struct del_virt_guestpart *delparams) >> >> i = virtpci_device_del(NULL /*no parent bus */, VIRTHBA_TYPE, >> &scsi.wwnn, NULL); >> - if (i) { >> + if (i) >> return 1; >> - } >> - return 0; >> + else >> + return 0; > No, now this will introduce a new checkpatch warning that "else is not > required after return". why did you introduce this "else"? I did this so that the code is more readable and understandable, I checked and checkpatch didn't call this out , so its clean. Otherwise the above code looks like this if(i) return 1; return 0; > > regards > sudip > --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue 2015-07-01 7:36 ` Sohny Thomas @ 2015-07-01 8:01 ` Julia Lawall 2015-07-01 9:20 ` Sohny Thomas 2015-07-01 8:02 ` Sudip Mukherjee 1 sibling, 1 reply; 9+ messages in thread From: Julia Lawall @ 2015-07-01 8:01 UTC (permalink / raw) To: Sohny Thomas Cc: Sudip Mukherjee, benjamin.romer, david.kershner, bryan.thompson, erik.arfvidson, devel, gregkh, sparmaintainer, kernel-janitors, linux-kernel On Wed, 1 Jul 2015, Sohny Thomas wrote: > Thanks for review, my answers inline > > On 01-07-2015 12:27, Sudip Mukherjee wrote: > > On Wed, Jul 01, 2015 at 03:05:45AM +0530, Sohny Thomas wrote: > > > > > > FIX 2 unnecessary braces found by checkpatch.pl > > > > > > Signed-off-by: Sohny Thomas <sohnythomas@zoho.com> > > > --- > > > drivers/staging/unisys/virtpci/virtpci.c | 11 ++++++----- > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/staging/unisys/virtpci/virtpci.c > > > b/drivers/staging/unisys/virtpci/virtpci.c > > > index d5ad017..f3674de 100644 > > > --- a/drivers/staging/unisys/virtpci/virtpci.c > > > +++ b/drivers/staging/unisys/virtpci/virtpci.c > > > @@ -190,9 +190,10 @@ static int write_vbus_chp_info(struct > > > spar_vbus_channel_protocol *chan, > > > return -1; > > > > > > off = sizeof(struct channel_header) + chan->hdr_info.chp_info_offset; > > > - if (chan->hdr_info.chp_info_offset == 0) { > > > + > > > + if (chan->hdr_info.chp_info_offset == 0) > > > return -1; > > > - } > > > + > > why you are inserting new line here? > I did it so that its readable, will remove it if not required > > > > > memcpy(((u8 *)(chan)) + off, info, sizeof(*info)); > > > return 0; > > > } > > > @@ -484,10 +485,10 @@ static int delete_vhba(struct del_virt_guestpart > > > *delparams) > > > > > > i = virtpci_device_del(NULL /*no parent bus */, VIRTHBA_TYPE, > > > &scsi.wwnn, NULL); > > > - if (i) { > > > + if (i) > > > return 1; > > > - } > > > - return 0; > > > + else > > > + return 0; > > No, now this will introduce a new checkpatch warning that "else is not > > required after return". why did you introduce this "else"? > I did this so that the code is more readable and understandable, I checked and > checkpatch didn't call this out , so its clean. > > Otherwise the above code looks like this > > if(i) > return 1; > return 0; That looks fine. I haven't looked at the code in detail. Is it normal that the return values seem to be 0 1 and -1? Which values represent success and which represent an error? It is nicer to have the errors under if and success as a direct return at the end. julia ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue 2015-07-01 8:01 ` Julia Lawall @ 2015-07-01 9:20 ` Sohny Thomas 2015-07-01 9:35 ` Julia Lawall 0 siblings, 1 reply; 9+ messages in thread From: Sohny Thomas @ 2015-07-01 9:20 UTC (permalink / raw) To: Julia Lawall Cc: Sudip Mukherjee, benjamin.romer, david.kershner, bryan.thompson, erik.arfvidson, devel, gregkh, sparmaintainer, kernel-janitors, linux-kernel >>>> i = virtpci_device_del(NULL /*no parent bus */, VIRTHBA_TYPE, >>>> &scsi.wwnn, NULL); >>>> - if (i) { >>>> + if (i) >>>> return 1; >>>> - } >>>> - return 0; >>>> + else >>>> + return 0; >>> No, now this will introduce a new checkpatch warning that "else is not >>> required after return". why did you introduce this "else"? >> I did this so that the code is more readable and understandable, I checked and >> checkpatch didn't call this out , so its clean. >> >> Otherwise the above code looks like this >> >> if(i) >> return 1; >> return 0; > > That looks fine. > > I haven't looked at the code in detail. Is it normal that the return > values seem to be 0 1 and -1? Which values represent success and which > represent an error? It is nicer to have the errors under if and success > as a direct return at the end. Here in this driver directory, return 1 means SUCCESS and return 0 means FAILURE So you mean my code change is fine? > > julia > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue 2015-07-01 9:20 ` Sohny Thomas @ 2015-07-01 9:35 ` Julia Lawall 0 siblings, 0 replies; 9+ messages in thread From: Julia Lawall @ 2015-07-01 9:35 UTC (permalink / raw) To: Sohny Thomas Cc: Julia Lawall, Sudip Mukherjee, benjamin.romer, david.kershner, bryan.thompson, erik.arfvidson, devel, gregkh, sparmaintainer, kernel-janitors, linux-kernel On Wed, 1 Jul 2015, Sohny Thomas wrote: > >>>> i = virtpci_device_del(NULL /*no parent bus */, VIRTHBA_TYPE, > >>>> &scsi.wwnn, NULL); > >>>> - if (i) { > >>>> + if (i) > >>>> return 1; > >>>> - } > >>>> - return 0; > >>>> + else > >>>> + return 0; > >>> No, now this will introduce a new checkpatch warning that "else is not > >>> required after return". why did you introduce this "else"? > >> I did this so that the code is more readable and understandable, I checked and > >> checkpatch didn't call this out , so its clean. > >> > >> Otherwise the above code looks like this > >> > >> if(i) > >> return 1; > >> return 0; > > > > That looks fine. > > > > I haven't looked at the code in detail. Is it normal that the return > > values seem to be 0 1 and -1? Which values represent success and which > > represent an error? It is nicer to have the errors under if and success > > as a direct return at the end. > Here in this driver directory, return 1 means SUCCESS and return 0 means FAILURE What is -1? > So you mean my code change is fine? I think it would be best to have the success case that is not under an if. So if (!i) return 0; return 1; I guess some day the driver would need more normal error codes? julia ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue 2015-07-01 7:36 ` Sohny Thomas 2015-07-01 8:01 ` Julia Lawall @ 2015-07-01 8:02 ` Sudip Mukherjee 2015-07-01 8:34 ` Sohny Thomas 1 sibling, 1 reply; 9+ messages in thread From: Sudip Mukherjee @ 2015-07-01 8:02 UTC (permalink / raw) To: Sohny Thomas Cc: benjamin.romer, david.kershner, bryan.thompson, erik.arfvidson, devel, gregkh, sparmaintainer, kernel-janitors, linux-kernel On Wed, Jul 01, 2015 at 01:06:48PM +0530, Sohny Thomas wrote: <snip> > >No, now this will introduce a new checkpatch warning that "else is not > >required after return". why did you introduce this "else"? > I did this so that the code is more readable and understandable, I > checked and checkpatch didn't call this out , so its clean. > > Otherwise the above code looks like this > > if(i) > return 1; > return 0; you should update your tree. virtpci folder has been deleted from unisys driver. As you are using an old tree, maybe that explains why checkpatch is not giving the error. regards sudip ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue 2015-07-01 8:02 ` Sudip Mukherjee @ 2015-07-01 8:34 ` Sohny Thomas 2015-07-01 9:15 ` Sudip Mukherjee 0 siblings, 1 reply; 9+ messages in thread From: Sohny Thomas @ 2015-07-01 8:34 UTC (permalink / raw) To: Sudip Mukherjee Cc: benjamin.romer, david.kershner, bryan.thompson, erik.arfvidson, devel, gregkh, sparmaintainer, kernel-janitors, linux-kernel On Wednesday 01 July 2015 01:32 PM, Sudip Mukherjee wrote: > On Wed, Jul 01, 2015 at 01:06:48PM +0530, Sohny Thomas wrote: > <snip> >>> No, now this will introduce a new checkpatch warning that "else is not >>> required after return". why did you introduce this "else"? >> I did this so that the code is more readable and understandable, I >> checked and checkpatch didn't call this out , so its clean. >> >> Otherwise the above code looks like this >> >> if(i) >> return 1; >> return 0; > you should update your tree. virtpci folder has been deleted from > unisys driver. > As you are using an old tree, maybe that explains why checkpatch is not > giving the error. This is from linux-stable branch and I updated it just yesterday, so looks like the folders still there > > regards > sudip > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue 2015-07-01 8:34 ` Sohny Thomas @ 2015-07-01 9:15 ` Sudip Mukherjee 0 siblings, 0 replies; 9+ messages in thread From: Sudip Mukherjee @ 2015-07-01 9:15 UTC (permalink / raw) To: Sohny Thomas Cc: benjamin.romer, david.kershner, bryan.thompson, erik.arfvidson, devel, gregkh, sparmaintainer, kernel-janitors, linux-kernel On Wed, Jul 01, 2015 at 02:04:48PM +0530, Sohny Thomas wrote: > On Wednesday 01 July 2015 01:32 PM, Sudip Mukherjee wrote: > > On Wed, Jul 01, 2015 at 01:06:48PM +0530, Sohny Thomas wrote: > <snip> > This is from linux-stable branch and I updated it just yesterday, so looks like the folders still there if you are sending patches for staging then you should work with staging-testing tree. regards sudip ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-07-01 9:36 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-06-30 21:35 [PATCH] Staging: unisys: virtpci: fixed a brace coding style issue Sohny Thomas 2015-07-01 6:57 ` Sudip Mukherjee 2015-07-01 7:36 ` Sohny Thomas 2015-07-01 8:01 ` Julia Lawall 2015-07-01 9:20 ` Sohny Thomas 2015-07-01 9:35 ` Julia Lawall 2015-07-01 8:02 ` Sudip Mukherjee 2015-07-01 8:34 ` Sohny Thomas 2015-07-01 9:15 ` Sudip Mukherjee
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox