On Sun, 2011-10-09 at 07:56 -0400, Christoph Hellwig wrote:
> On Sun, Oct 09, 2011 at 08:56:33AM +0000, Nicholas A. Bellinger wrote:
> > + if ((cmd->iscsi_opcode != ISCSI_OP_SCSI_CMD) &&
> > + (cmd->iscsi_opcode != ISCSI_OP_SCSI_TMFUNC))
> > iscsit_release_cmd(cmd);
> > else
> > transport_generic_free_cmd(&cmd->se_cmd,
>
> This code block is duplicated all over, I think it should be factored
> into a little helper. Also there are superflous braces in the code
> above, e.g.
>
> void iscsit_free_cmd(struct iscsi_cmd *cmd)
> {
> switch (cmd->iscsi_opcode) {
> case ISCSI_OP_SCSI_CMD:
> case ISCSI_OP_SCSI_TMFUNC:
> iscsit_release_cmd(cmd);
> break;
> default:
> transport_generic_free_cmd(&cmd->se_cmd, 1);
> break;
> }
> }
>
Yes, I originally open coded these to ensure I got the exact opcodes
cases right for individual context. Using a iscsit_free_cmd() helper
with the extra (and in most cases unnecessary) checks is fine, and I
suppose is slightly cleaner than the opencoded variant.
Also just FYI, the iscsit_free_cmd() logic above is also inverted. ;)
> > + if (cmd->iscsi_opcode != ISCSI_OP_SCSI_CMD)
> > iscsit_release_cmd(cmd);
> > else
> > transport_generic_free_cmd(&cmd->se_cmd, 1);
>
> And this doesn't test for ISCSI_OP_SCSI_TMFUNC just because it can't
> happen here, because of oversight or another reason?
No, TMRs cannot be reassigned to a different iSCSI connection during
connection recovery, so this along with the other cases that do not
contain both ISCSI_OP_SCSI_CMD and ISCSI_OP_SCSI_TMFUNC checks where
originally correct.
Anyways, adding a iscsit_free_cmd() wrapper..
--nab
--
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
