On Mon, 2 Jan 2017 00:29:19 -0800, Christoph Hellwig wrote:
> > static int target_xcopy_parse_tiddesc_e4(struct se_cmd *se_cmd, struct xcopy_op *xop,
> > - unsigned char *p, bool src)
> > + unsigned char *p, int cscd_index)
>
> unsigned?
I've changed it to an unsigned short here and in the caller, to match
stdi/dtdi.
>
> > + if ((cscd_index != xop->stdi) && (cscd_index != xop->dtdi)) {
>
> drop the inner braces.
Dropped.
>
> > + pr_debug("XCOPY 0xe4: ignoring CSCD entry %d - neither src nor "
> > + "dest\n", cscd_index);
> > + return 0;
> > + }
> > +
> > + if (cscd_index == xop->stdi) {
> > memcpy(&xop->src_tid_wwn[0], &desc[8], XCOPY_NAA_IEEE_REGEX_LEN);
> > /*
> > * Determine if the source designator matches the local device
> > @@ -155,10 +161,15 @@ static int target_xcopy_parse_tiddesc_e4(struct se_cmd *se_cmd, struct xcopy_op
> > pr_debug("XCOPY 0xe4: Set xop->src_dev %p from source"
> > " received xop\n", xop->src_dev);
> > }
> > - } else {
> > + }
> > +
> > + if (cscd_index == xop->dtdi) {
>
> And that whole thing should probably become a switch statement.
I don't quite follow here - neither stdi, dtdi, nor cscd_index are
constants.
>
> > - if (xop->op_origin == XCOL_SOURCE_RECV_OP)
> > + if (xop->op_origin == XCOL_SOURCE_RECV_OP) {
> > rc = target_xcopy_locate_se_dev_e4(xop->dst_tid_wwn,
> > &xop->dst_dev);
> > - else
> > + } else if (xop->op_origin == XCOL_DEST_RECV_OP) {
> > rc = target_xcopy_locate_se_dev_e4(xop->src_tid_wwn,
> > &xop->src_dev);
> > + } else {
> > + pr_err("XCOPY CSCD descriptor IDs not found in CSCD list - "
> > + "stdi: %hu dtdi: %hu\n", xop->stdi, xop->dtdi);
> > + rc = -EINVAL;
> > + }
>
> Another candidate for a switch statement.
Done.
>
> > + if (rc <= 0) {
> > + goto out;
> > + }
>
> no need for the braces.
Dropped.
Cheers, David
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
