On Fri, 14 Oct 2005, Christoph Hellwig wrote:
> On Thu, Oct 13, 2005 at 04:16:04PM -0700, Andrew Vasquez wrote:
> > Signed-off-by: Andrew Vasquez <andrew.vasquez@xxxxxxxxxx>
>
> I don't like this at all. Even the first variant with the indirect
> function call seems better than this.
The indirect function call is still there -- the resultant code hasn't
changed with this patch. This patch simply insures that common
queueing code gets updated in one spot rather than two
(qla2x00_queuecommand() qla24xx_queuecommand()). The *only*
difference between the two was the call to
[qla2x00|qla24xx]_start_scsi(). So, beyond style, are there any other
objections.
> > +#define QLA_QUEUE_COMMAND(isp) \
> > +static int \
> > +isp##_queuecommand(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)) \
> > +{ \
> > + scsi_qla_host_t *ha = to_qla_host(cmd->device->host); \
> > + fc_port_t *fcport = (struct fc_port *) cmd->device->hostdata; \
> > + srb_t *sp; \
> > + int rval; \
> > + if (!fcport) { \
> > + cmd->result = DID_NO_CONNECT << 16; \
> > + goto fail_command; \
> > + } \
>
> While we're at it, I don't think cmd->device->hostdata could ever be
> NULL.
Yes, with the target_parent checks in scsi_alloc_target():
if (shost->transportt->target_parent) {
spin_lock_irqsave(shost->host_lock, flags);
parent = shost->transportt->target_parent(shost, channel, id);
spin_unlock_irqrestore(shost->host_lock, flags);
if (!parent)
return NULL;
}
you are correct. Yet another reason to collapse both -- change the
code in one place.
--
Andrew Vasquez
-
: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
