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

[Dan Carpenter <dan.carpenter@oracle.com>]

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