Re: [patch] target/iscsi: precedence bug in iscsit_set_dataout_sequence_values()

2012-10-03 Thread walter harms 


Am 02.10.2012 22:30, schrieb Nicholas A. Bellinger:
 On Tue, 2012-10-02 at 11:22 +0300, Dan Carpenter wrote:
 Clang warns about this bug:
 drivers/target/iscsi/iscsi_target_erl0.c:52:45: warning: operator '?:'
  has lower precedence than '+'; '+' will be evaluated first
  [-Wparentheses]

 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 ---
 Please review this very carefully because I haven't tested it.  It could
 be that the check should be:
 (data_done + data_length  FirstBurstLength) ? FirstBurstLength : 
 data_length);
 Instead of what I have which is:
 data_done + (data_length  FirstBurstLength ? FirstBurstLength : 
 data_length);

 diff --git a/drivers/target/iscsi/iscsi_target_erl0.c 
 b/drivers/target/iscsi/iscsi_target_erl0.c
 index 1a02016..2067efd 100644
 --- a/drivers/target/iscsi/iscsi_target_erl0.c
 +++ b/drivers/target/iscsi/iscsi_target_erl0.c
 @@ -48,9 +48,9 @@ void iscsit_set_dataout_sequence_values(
  if (cmd-unsolicited_data) {
  cmd-seq_start_offset = cmd-write_data_done;
  cmd-seq_end_offset = (cmd-write_data_done +
 -(cmd-se_cmd.data_length 
 - conn-sess-sess_ops-FirstBurstLength) ?
 -conn-sess-sess_ops-FirstBurstLength : 
 cmd-se_cmd.data_length);
 +((cmd-se_cmd.data_length 
 +  conn-sess-sess_ops-FirstBurstLength) ?
 + conn-sess-sess_ops-FirstBurstLength : 
 cmd-se_cmd.data_length));
  return;
  }
  
 --
 
 This is indeed the original intention and your patch is correct, so
 applied to for-next.
 
 Thank you!
 
 --nab
 

please consider rewriting this into an if ... else ... statement.
See my comments and Dan's reply on this.

re,
 wh
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch] target/iscsi: precedence bug in iscsit_set_dataout_sequence_values()

2012-10-02 Thread Dan Carpenter 
Clang warns about this bug:
drivers/target/iscsi/iscsi_target_erl0.c:52:45: warning: operator '?:'
has lower precedence than '+'; '+' will be evaluated first
[-Wparentheses]

Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
---
Please review this very carefully because I haven't tested it.  It could
be that the check should be:
(data_done + data_length  FirstBurstLength) ? FirstBurstLength : data_length);
Instead of what I have which is:
data_done + (data_length  FirstBurstLength ? FirstBurstLength : data_length);

diff --git a/drivers/target/iscsi/iscsi_target_erl0.c 
b/drivers/target/iscsi/iscsi_target_erl0.c
index 1a02016..2067efd 100644
--- a/drivers/target/iscsi/iscsi_target_erl0.c
+++ b/drivers/target/iscsi/iscsi_target_erl0.c
@@ -48,9 +48,9 @@ void iscsit_set_dataout_sequence_values(
if (cmd-unsolicited_data) {
cmd-seq_start_offset = cmd-write_data_done;
cmd-seq_end_offset = (cmd-write_data_done +
-   (cmd-se_cmd.data_length 
-conn-sess-sess_ops-FirstBurstLength) ?
-   conn-sess-sess_ops-FirstBurstLength : 
cmd-se_cmd.data_length);
+   ((cmd-se_cmd.data_length 
+ conn-sess-sess_ops-FirstBurstLength) ?
+conn-sess-sess_ops-FirstBurstLength : 
cmd-se_cmd.data_length));
return;
}
 
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] target/iscsi: precedence bug in iscsit_set_dataout_sequence_values()

2012-10-02 Thread walter harms 


Am 02.10.2012 10:22, schrieb Dan Carpenter:
 Clang warns about this bug:
 drivers/target/iscsi/iscsi_target_erl0.c:52:45: warning: operator '?:'
   has lower precedence than '+'; '+' will be evaluated first
   [-Wparentheses]
 
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 ---
 Please review this very carefully because I haven't tested it.  It could
 be that the check should be:
 (data_done + data_length  FirstBurstLength) ? FirstBurstLength : 
 data_length);
 Instead of what I have which is:
 data_done + (data_length  FirstBurstLength ? FirstBurstLength : data_length);
 
 diff --git a/drivers/target/iscsi/iscsi_target_erl0.c 
 b/drivers/target/iscsi/iscsi_target_erl0.c
 index 1a02016..2067efd 100644
 --- a/drivers/target/iscsi/iscsi_target_erl0.c
 +++ b/drivers/target/iscsi/iscsi_target_erl0.c
 @@ -48,9 +48,9 @@ void iscsit_set_dataout_sequence_values(
   if (cmd-unsolicited_data) {
   cmd-seq_start_offset = cmd-write_data_done;
   cmd-seq_end_offset = (cmd-write_data_done +
 - (cmd-se_cmd.data_length 
 -  conn-sess-sess_ops-FirstBurstLength) ?
 - conn-sess-sess_ops-FirstBurstLength : 
 cmd-se_cmd.data_length);
 + ((cmd-se_cmd.data_length 
 +   conn-sess-sess_ops-FirstBurstLength) ?
 +  conn-sess-sess_ops-FirstBurstLength : 
 cmd-se_cmd.data_length));
   return;
   }
  

the ?: operator is nice but at a certain length is starts to become unreadable,
the end is normally calculated by end= start+len; Therefor i suggest:

if ( cmd-se_cmd.data_length  conn-sess-sess_ops-FirstBurstLength )
   cmd-seq_end_offset = cmd-seq_start_offset + 
conn-sess-sess_ops-FirstBurstLength;
else
   cmd-seq_end_offset = cmd-seq_start_offset + cmd-se_cmd.data_length;

just my 2 cents,
re,
 wh
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] target/iscsi: precedence bug in iscsit_set_dataout_sequence_values()

2012-10-02 Thread Dan Carpenter 
On Tue, Oct 02, 2012 at 01:48:57PM +0200, walter harms wrote:
 
 
 Am 02.10.2012 10:22, schrieb Dan Carpenter:
  Clang warns about this bug:
  drivers/target/iscsi/iscsi_target_erl0.c:52:45: warning: operator '?:'
  has lower precedence than '+'; '+' will be evaluated first
  [-Wparentheses]
  
  Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
  ---
  Please review this very carefully because I haven't tested it.  It could
  be that the check should be:
  (data_done + data_length  FirstBurstLength) ? FirstBurstLength : 
  data_length);
  Instead of what I have which is:
  data_done + (data_length  FirstBurstLength ? FirstBurstLength : 
  data_length);
  
  diff --git a/drivers/target/iscsi/iscsi_target_erl0.c 
  b/drivers/target/iscsi/iscsi_target_erl0.c
  index 1a02016..2067efd 100644
  --- a/drivers/target/iscsi/iscsi_target_erl0.c
  +++ b/drivers/target/iscsi/iscsi_target_erl0.c
  @@ -48,9 +48,9 @@ void iscsit_set_dataout_sequence_values(
  if (cmd-unsolicited_data) {
  cmd-seq_start_offset = cmd-write_data_done;
  cmd-seq_end_offset = (cmd-write_data_done +
  -   (cmd-se_cmd.data_length 
  -conn-sess-sess_ops-FirstBurstLength) ?
  -   conn-sess-sess_ops-FirstBurstLength : 
  cmd-se_cmd.data_length);
  +   ((cmd-se_cmd.data_length 
  + conn-sess-sess_ops-FirstBurstLength) ?
  +conn-sess-sess_ops-FirstBurstLength : 
  cmd-se_cmd.data_length));
  return;
  }
   
 
 the ?: operator is nice but at a certain length is starts to become 
 unreadable,
 the end is normally calculated by end= start+len; Therefor i suggest:
 
 if ( cmd-se_cmd.data_length  conn-sess-sess_ops-FirstBurstLength )
cmd-seq_end_offset = cmd-seq_start_offset + 
 conn-sess-sess_ops-FirstBurstLength;
 else
cmd-seq_end_offset = cmd-seq_start_offset + cmd-se_cmd.data_length;
 

Yeah.  It's not beautiful.

Doing a struct iscsi_sess_ops *ops = conn-sess-sess_ops; at the
start of the function would help as well.

regards,
dan carpenter

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] target/iscsi: precedence bug in iscsit_set_dataout_sequence_values()

2012-10-02 Thread Nicholas A. Bellinger 
On Tue, 2012-10-02 at 11:22 +0300, Dan Carpenter wrote:
 Clang warns about this bug:
 drivers/target/iscsi/iscsi_target_erl0.c:52:45: warning: operator '?:'
   has lower precedence than '+'; '+' will be evaluated first
   [-Wparentheses]
 
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 ---
 Please review this very carefully because I haven't tested it.  It could
 be that the check should be:
 (data_done + data_length  FirstBurstLength) ? FirstBurstLength : 
 data_length);
 Instead of what I have which is:
 data_done + (data_length  FirstBurstLength ? FirstBurstLength : data_length);
 
 diff --git a/drivers/target/iscsi/iscsi_target_erl0.c 
 b/drivers/target/iscsi/iscsi_target_erl0.c
 index 1a02016..2067efd 100644
 --- a/drivers/target/iscsi/iscsi_target_erl0.c
 +++ b/drivers/target/iscsi/iscsi_target_erl0.c
 @@ -48,9 +48,9 @@ void iscsit_set_dataout_sequence_values(
   if (cmd-unsolicited_data) {
   cmd-seq_start_offset = cmd-write_data_done;
   cmd-seq_end_offset = (cmd-write_data_done +
 - (cmd-se_cmd.data_length 
 -  conn-sess-sess_ops-FirstBurstLength) ?
 - conn-sess-sess_ops-FirstBurstLength : 
 cmd-se_cmd.data_length);
 + ((cmd-se_cmd.data_length 
 +   conn-sess-sess_ops-FirstBurstLength) ?
 +  conn-sess-sess_ops-FirstBurstLength : 
 cmd-se_cmd.data_length));
   return;
   }
  
 --

This is indeed the original intention and your patch is correct, so
applied to for-next.

Thank you!

--nab

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html