On Mon, Jun 21, 2010 at 5:35 PM, Kenneth R Westerback <[email protected]> wrote: > --- dev/ic/adw.c 20 May 2010 00:55:17 -0000 1.44 > +++ dev/ic/adw.c 22 Jun 2010 00:31:56 -0000 > @@ -690,10 +689,9 @@ adw_build_req(xs, ccb, flags) > * For wide boards a CDB length maximum of 16 bytes > * is supported. > */ > - bcopy(xs->cmd, &scsiqp->cdb, ((scsiqp->cdb_len = xs->cmdlen) <= 12)? > - xs->cmdlen : 12 ); > - if(xs->cmdlen > 12) > - bcopy(&(xs->cmd[12]), &scsiqp->cdb16, xs->cmdlen - 12); > + scsiqp->cdb_len = xs->cmdlen; > + bcopy(xs->cmd, &scsiqp->cdb, 12); > + bcopy(&(xs->cmd->bytes[11]), &scsiqp->cdb16, 4); > > scsiqp->target_id = sc_link->target; > scsiqp->target_lun = sc_link->lun;
I somewhat preferred my diff because: 1. It's immediately obvious that (caddr_t)xs->cmd + 12 is skipping the first 12 bytes of xs->cmd, whereas &xs->cmd->bytes[11] requires knowing the layout of scsi_generic. 2. We already know how many bytes are actually needed, so it's unnecessary to copy more, and arguably it's an abstraction violation to assume more than xs->cmdlen bytes are available in xs->cmd. (In practice, it always points to a 16-byte buffer and we should kill the antiquated cmd/cmdspace distinction anyway.) That said, I don't see any non-stylistic issues with your diff. Also, the arch/vax/dev/sii.c changes look correct to me as well.
