On Sat, 2011-11-05 at 11:34 +0100, Christoph Hellwig wrote: > On Fri, Nov 04, 2011 at 01:40:00PM -0700, Nicholas A. Bellinger wrote: > > Well, I think the removal of target specific returns universally cleans > > things up (more) from the perspective of fabric module code. > > It's for sure better than before, no argument there. > > > I don't > > really see a reason for doing this anymore if cmd->scsi_sense_reason is > > set directly by target core, and if fabric module code is just checking > > for non zero or less than zero return values to determine exception > > status. > > If it is a bool, make it a bool and don't confuse people with the > errno values. But from looking at the code it seems like we don't > even need that - if you add TCM_SUCCESS and TCM_QUEUE_FULL values > to enum tcm_sense_reason_table we could express all possible return > values from the I/O path that way. > > If you make sure TCM_SUCCESS is 0 you still have your if (!ret) check > for error path, but we get a much clearer picture of what is going on. > I'd really like to avoid using tcm_sense_reason_table for function return values, or making external fabric code responsible for setting ->scsi_sense_reason by propagating up these values as returns. Also, tcm_sense_reason_table implies CHECK_CONDITION status, and there are cases where we only want to setting ->scsi_status. Although TCM_RESERVATION_CONFLICT is currently abusing this a little.. ;) Anyways, adding new return codes doesn't really seem to add much here either. Do you have a strong objecting to sticking with errno as long as the fabrics are not response for decoding specific failure status..? Eg: core is responsible for setting ->scsi_status or ->scsi_sense_reason, and transport_handle_cdb_direct() is response for queuing up response via transport_generic_request_failure()..? transport_generic_allocate_tasks() is the one case (due to iscsi-target handling of immediate data) where the fabric is response for calling transport_send_check_condition_and_sense() or queueing up non GOOD status directly.. > > > > + if (!task->task_error_status) > > > > + cmd->scsi_sense_reason = > > > > + TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; > > > > + > > > > > > task_error_status is never set after your patch. > > > > > > > Whoops. I'm not sure if we really have a functional need for > > task->task_error_status anymore, but it still might be useful for > > debugging purposes. I'll look at dropping this as well. > > I don't think we do. Even before your patch is was used as a rather > whacky boolean during I/O completion. <nod> -- 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
