From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43088) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xjnry-0003NE-BH for qemu-devel@nongnu.org; Thu, 30 Oct 2014 07:26:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xjnrs-0002jI-9j for qemu-devel@nongnu.org; Thu, 30 Oct 2014 07:26:22 -0400 Received: from cantor2.suse.de ([195.135.220.15]:33366 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xjnrr-0002j0-UM for qemu-devel@nongnu.org; Thu, 30 Oct 2014 07:26:16 -0400 Message-ID: <54522054.1010902@suse.de> Date: Thu, 30 Oct 2014 12:26:12 +0100 From: Hannes Reinecke MIME-Version: 1.0 References: <1414569232-21357-1-git-send-email-hare@suse.de> <1414569232-21357-4-git-send-email-hare@suse.de> <54521520.4000805@redhat.com> In-Reply-To: <54521520.4000805@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 03/17] scsi: Rename scsi_cdb_length() to scsi_xfer_length() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org Cc: Alexander Graf , Nic Bellinger , Andreas Faerber On 10/30/2014 11:38 AM, Paolo Bonzini wrote: >=20 >=20 > On 10/29/2014 08:53 AM, Hannes Reinecke wrote: >> scsi_cdb_length() does not return the length of the cdb, but >> the transfersize encoded in the cdb. So rename it to scsi_xfer_length(= ) >> and add a new scsi_cdb_length() which actually does return the >> length of the cdb. >=20 > This makes sense, but it messes up the function names even more. We no= w > have ata_passthrough_*_xfer_size, scsi_xfer_length, scsi_req_length, > scsi_req_*_length. Let's standardize on scsi_*_xfer: >=20 Okay. > diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c > index 7763847..3eccb1b 100644 > --- a/hw/scsi/scsi-bus.c > +++ b/hw/scsi/scsi-bus.c > @@ -820,7 +820,7 @@ static int ata_passthrough_xfer_unit(SCSIDevice *de= v, uint8_t *buf) > return xfer_unit; > } > =20 > -static int ata_passthrough_12_xfer_size(SCSIDevice *dev, uint8_t *buf) > +static int ata_passthrough_12_xfer(SCSIDevice *dev, uint8_t *buf) > { > int length =3D buf[2] & 0x3; > int xfer; > @@ -843,7 +843,7 @@ static int ata_passthrough_12_xfer_size(SCSIDevice = *dev, uint8_t *buf) > return xfer * unit; > } > =20 > -static int ata_passthrough_16_xfer_size(SCSIDevice *dev, uint8_t *buf) > +static int ata_passthrough_16_xfer(SCSIDevice *dev, uint8_t *buf) > { > int extend =3D buf[1] & 0x1; > int length =3D buf[2] & 0x3; > @@ -874,11 +874,11 @@ uint32_t scsi_data_cdb_length(uint8_t *buf) > if ((buf[0] >> 5) =3D=3D 0 && buf[4] =3D=3D 0) { > return 256; > } else { > - return scsi_xfer_length(buf); > + return scsi_cdb_xfer(buf); > } > } > =20 > -uint32_t scsi_xfer_length(uint8_t *buf) > +uint32_t scsi_cdb_xfer(uint8_t *buf) > { > switch (buf[0] >> 5) { > case 0: > @@ -899,9 +899,9 @@ uint32_t scsi_xfer_length(uint8_t *buf) > } > } > =20 > -static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t = *buf) > +static int scsi_req_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8_t *b= uf) > { > - cmd->xfer =3D scsi_xfer_length(buf); > + cmd->xfer =3D scsi_cdb_xfer(buf); > switch (buf[0]) { > case TEST_UNIT_READY: > case REWIND: > @@ -1032,17 +1032,17 @@ static int scsi_req_length(SCSICommand *cmd, SC= SIDevice *dev, uint8_t *buf) > /* BLANK command of MMC */ > cmd->xfer =3D 0; > } else { > - cmd->xfer =3D ata_passthrough_12_xfer_size(dev, buf); > + cmd->xfer =3D ata_passthrough_12_xfer(dev, buf); > } > break; > case ATA_PASSTHROUGH_16: > - cmd->xfer =3D ata_passthrough_16_xfer_size(dev, buf); > + cmd->xfer =3D ata_passthrough_16_xfer(dev, buf); > break; > } > return 0; > } > =20 > -static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, u= int8_t *buf) > +static int scsi_req_stream_xfer(SCSICommand *cmd, SCSIDevice *dev, uin= t8_t *buf) > { > switch (buf[0]) { > /* stream commands */ > @@ -1097,12 +1097,12 @@ static int scsi_req_stream_length(SCSICommand *= cmd, SCSIDevice *dev, uint8_t *bu > break; > /* generic commands */ > default: > - return scsi_req_length(cmd, dev, buf); > + return scsi_req_xfer(cmd, dev, buf); > } > return 0; > } > =20 > -static int scsi_req_medium_changer_length(SCSICommand *cmd, SCSIDevice= *dev, uint8_t *buf) > +static int scsi_req_medium_changer_xfer(SCSICommand *cmd, SCSIDevice *= dev, uint8_t *buf) > { > switch (buf[0]) { > /* medium changer commands */ > @@ -1119,7 +1119,7 @@ static int scsi_req_medium_changer_length(SCSICom= mand *cmd, SCSIDevice *dev, uin > =20 > /* generic commands */ > default: > - return scsi_req_length(cmd, dev, buf); > + return scsi_req_xfer(cmd, dev, buf); > } > return 0; > } > @@ -1240,13 +1240,13 @@ int scsi_req_parse_cdb(SCSIDevice *dev, SCSICom= mand *cmd, uint8_t *buf) > =20 > switch (dev->type) { > case TYPE_TAPE: > - rc =3D scsi_req_stream_length(cmd, dev, buf); > + rc =3D scsi_req_stream_xfer(cmd, dev, buf); > break; > case TYPE_MEDIUM_CHANGER: > - rc =3D scsi_req_medium_changer_length(cmd, dev, buf); > + rc =3D scsi_req_medium_changer_xfer(cmd, dev, buf); > break; > default: > - rc =3D scsi_req_length(cmd, dev, buf); > + rc =3D scsi_req_xfer(cmd, dev, buf); > break; > } > =20 >=20 >> With that DEBUG_SCSI can now display the correct CDB buffer. >=20 > Why would req->cmd.len be wrong though? Are you masking another bug? >=20 No. I'm _fixing_ a bug. scsi_disk.c:scsi_new_request() is just calling scsi_req_alloc(), which does not set cmd.len. Yet later on scsi_new_request() uses cmd.len to print out the CDB, which at the time isn't initialized. Cheers, Hannes --=20 Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg GF: J. Hawn, J. Guild, F. Imend=F6rffer, HRB 16746 (AG N=FCrnberg)