Skip to contentSkip to conversation

Staging: unisys: virtpci: fixed a brace coding style issue

PATCH In review Sohny Thomas 3 participants 9 messages Jun 30, 2015 – Jul 1, 2015

Subsystems (2)
Staging Kernel janitors
lore
Share & tools
lore

Sohny Thomas

author
#0 lore raw

PATCH 1/1 Staging: unisys: virtpci: fixed a brace coding style issue

FIX 2 unnecessary braces found by checkpatch.pl

Signed-off-by Sohny Thomas <[email protected]>
---
 drivers/staging/unisys/virtpci/virtpci.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
drivers/staging/unisys/virtpci/virtpci.c +6 -5
index d5ad017..f3674de 100644
@@ -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
#1 lore raw
Quoted 13 lines · Sohny Thomas

FIX 2 unnecessary braces found by checkpatch.pl

Signed-off-by Sohny Thomas <[email protected]>
---
 drivers/staging/unisys/virtpci/virtpci.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
drivers/staging/unisys/virtpci/virtpci.c +0 -0
index d5ad017..f3674de 100644

drivers/staging/unisys/virtpci/virtpci.c static int write_vbus_chp_info(struct spar_vbus_channel_protocol *chan, · line 196

Quoted 9 lines
drivers/staging/unisys/virtpci/virtpci.c +3 -2
@@ -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?

Quoted 4 lines
drivers/staging/unisys/virtpci/virtpci.c +0 -0
 	memcpy(((u8 *)(chan)) + off, info, sizeof(*info));
 	return 0;
 }

drivers/staging/unisys/virtpci/virtpci.c static int delete_vhba(struct del_virt_guestpart *delparams) · line 491

Quoted 10 lines
drivers/staging/unisys/virtpci/virtpci.c +3 -3
@@ -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

author
#2 lore raw

Thanks for review, my answers inline

Quoted 16 lines · Sudip Mukherjee
Quoted 15 lines · Sohny Thomas

FIX 2 unnecessary braces found by checkpatch.pl

Signed-off-by Sohny Thomas <[email protected]>
---
  drivers/staging/unisys/virtpci/virtpci.c | 11 ++++++-----
  1 file changed, 6 insertions(+), 5 deletions(-)
drivers/staging/unisys/virtpci/virtpci.c +0 -0
index d5ad017..f3674de 100644
@@ -190,9 +190,10 @@ static int write_vbus_chp_info(struct spar_vbus_channel_protocol *chan,
  		return -1;
Quoted 8 lines

drivers/staging/unisys/virtpci/virtpci.c static int write_vbus_chp_info(struct spar_vbus_channel_protocol *chan, · line 196

Quoted 7 lines
drivers/staging/unisys/virtpci/virtpci.c +3 -2
  	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

Quoted 6 lines
Quoted 6 lines
drivers/staging/unisys/virtpci/virtpci.c +0 -0
  	memcpy(((u8 *)(chan)) + off, info, sizeof(*info));
  	return 0;
  }

@@ -484,10 +485,10 @@ static int delete_vhba(struct del_virt_guestpart *delparams)
Quoted 11 lines

drivers/staging/unisys/virtpci/virtpci.c static int delete_vhba(struct del_virt_guestpart *delparams) · line 491

Quoted 9 lines
drivers/staging/unisys/virtpci/virtpci.c +3 -3
  	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;

Quoted 4 lines

regards
sudip

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

#3 lore raw
Quoted 48 lines · Sohny Thomas

Thanks for review, my answers inline

Quoted 18 lines · Sudip Mukherjee
Quoted 17 lines · Sohny Thomas

FIX 2 unnecessary braces found by checkpatch.pl

Signed-off-by Sohny Thomas <[email protected]>
---
  drivers/staging/unisys/virtpci/virtpci.c | 11 ++++++-----
  1 file changed, 6 insertions(+), 5 deletions(-)
drivers/staging/unisys/virtpci/virtpci.c +0 -0
index d5ad017..f3674de 100644
@@ -190,9 +190,10 @@ static int write_vbus_chp_info(struct
 spar_vbus_channel_protocol *chan,
  		return -1;
Quoted 8 lines

drivers/staging/unisys/virtpci/virtpci.c static int write_vbus_chp_info(struct spar_vbus_channel_protocol *chan, · line 196

Quoted 7 lines
drivers/staging/unisys/virtpci/virtpci.c +3 -2
  	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

Quoted 7 lines
Quoted 7 lines
drivers/staging/unisys/virtpci/virtpci.c +0 -0
  	memcpy(((u8 *)(chan)) + off, info, sizeof(*info));
  	return 0;

  }
drivers/staging/unisys/virtpci/virtpci.c +0 -0
@@ -484,10 +485,10 @@ static int delete_vhba(struct del_virt_guestpart
 *delparams)
Quoted 11 lines

drivers/staging/unisys/virtpci/virtpci.c static int delete_vhba(struct del_virt_guestpart *delparams) · line 491

Quoted 9 lines
drivers/staging/unisys/virtpci/virtpci.c +3 -3
  	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"?

Quoted 8 lines

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

author
#4 lore raw
Quoted 20 lines
Quoted 11 lines
Quoted 11 lines

drivers/staging/unisys/virtpci/virtpci.c static int delete_vhba(struct del_virt_guestpart *delparams) · line 491

Quoted 9 lines

i = virtpci_device_del(NULL /*no parent bus */, VIRTHBA_TYPE,
&scsi.wwnn, NULL);

drivers/staging/unisys/virtpci/virtpci.c +3 -3
-	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"?

Quoted 8 lines

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;

Quoted 6 lines

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?

Quoted 3 lines

julia

#5 lore raw
Quoted 20 lines · Sohny Thomas
Quoted 20 lines
Quoted 19 lines
Quoted 11 lines

drivers/staging/unisys/virtpci/virtpci.c static int delete_vhba(struct del_virt_guestpart *delparams) · line 491

Quoted 9 lines

i = virtpci_device_del(NULL /*no parent bus */, VIRTHBA_TYPE,
&scsi.wwnn, NULL);

drivers/staging/unisys/virtpci/virtpci.c +3 -3
-	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;

Quoted 7 lines
Quoted 6 lines

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?

Quoted 1 line

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

#6 lore raw
In reply to Sohny Thomas

On Wed, Jul 01, 2015 at 01:06:48PM +0530, Sohny Thomas wrote:
<snip>

Quoted 10 lines
Quoted 2 lines

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

author
#7 lore raw
Quoted 9 lines · Sudip Mukherjee

On Wed, Jul 01, 2015 at 01:06:48PM +0530, Sohny Thomas wrote:
<snip>

Quoted 7 lines
Quoted 2 lines

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

Quoted 7 lines
Quoted 3 lines

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

Quoted 4 lines

regards
sudip

#8 lore raw
Quoted 3 lines · Sohny Thomas
Quoted 1 line · Sudip Mukherjee

On Wed, Jul 01, 2015 at 01:06:48PM +0530, Sohny Thomas wrote:

<snip>

Quoted 1 line

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