* [patch] target/iscsi: precedence bug in iscsit_set_dataout_sequence_values()
@ 2012-10-02 8:22 Dan Carpenter
2012-10-02 11:48 ` walter harms
2012-10-02 20:30 ` Nicholas A. Bellinger
0 siblings, 2 replies; 5+ messages in thread
From: Dan Carpenter @ 2012-10-02 8:22 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Andy Grover, Christoph Hellwig, linux-scsi, target-devel,
kernel-janitors
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.carpenter@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;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [patch] target/iscsi: precedence bug in iscsit_set_dataout_sequence_values()
2012-10-02 8:22 [patch] target/iscsi: precedence bug in iscsit_set_dataout_sequence_values() Dan Carpenter
@ 2012-10-02 11:48 ` walter harms
2012-10-02 12:27 ` Dan Carpenter
2012-10-02 20:30 ` Nicholas A. Bellinger
1 sibling, 1 reply; 5+ messages in thread
From: walter harms @ 2012-10-02 11:48 UTC (permalink / raw)
To: Dan Carpenter
Cc: Nicholas A. Bellinger, Andy Grover, Christoph Hellwig, linux-scsi,
target-devel, kernel-janitors
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.carpenter@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] target/iscsi: precedence bug in iscsit_set_dataout_sequence_values()
2012-10-02 11:48 ` walter harms
@ 2012-10-02 12:27 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2012-10-02 12:27 UTC (permalink / raw)
To: walter harms
Cc: Nicholas A. Bellinger, Andy Grover, Christoph Hellwig, linux-scsi,
target-devel, kernel-janitors
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.carpenter@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] target/iscsi: precedence bug in iscsit_set_dataout_sequence_values()
2012-10-02 8:22 [patch] target/iscsi: precedence bug in iscsit_set_dataout_sequence_values() Dan Carpenter
2012-10-02 11:48 ` walter harms
@ 2012-10-02 20:30 ` Nicholas A. Bellinger
2012-10-03 8:00 ` walter harms
1 sibling, 1 reply; 5+ messages in thread
From: Nicholas A. Bellinger @ 2012-10-02 20:30 UTC (permalink / raw)
To: Dan Carpenter
Cc: Andy Grover, Christoph Hellwig, linux-scsi, target-devel,
kernel-janitors
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.carpenter@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] target/iscsi: precedence bug in iscsit_set_dataout_sequence_values()
2012-10-02 20:30 ` Nicholas A. Bellinger
@ 2012-10-03 8:00 ` walter harms
0 siblings, 0 replies; 5+ messages in thread
From: walter harms @ 2012-10-03 8:00 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Dan Carpenter, Andy Grover, Christoph Hellwig, linux-scsi,
target-devel, kernel-janitors
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.carpenter@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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-10-03 8:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-02 8:22 [patch] target/iscsi: precedence bug in iscsit_set_dataout_sequence_values() Dan Carpenter
2012-10-02 11:48 ` walter harms
2012-10-02 12:27 ` Dan Carpenter
2012-10-02 20:30 ` Nicholas A. Bellinger
2012-10-03 8:00 ` walter harms
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox