Re: [Qemu-ppc] [PATCH] spapr_vscsi: remove duplicate condition check

[Paolo Bonzini]

Il 24/03/2014 18:19, Prasad Joshi ha scritto:
> On Mon, Mar 24, 2014 at 9:15 PM, Paolo Bonzini <address@hidden> wrote:
> > Il 24/03/2014 16:44, Prasad Joshi ha scritto:
> >
> > > Signed-off-by: Prasad Joshi <address@hidden>
> > > ---
> > >  hw/scsi/spapr_vscsi.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> > > index 34478f0..d4ada4f 100644
> > > --- a/hw/scsi/spapr_vscsi.c
> > > +++ b/hw/scsi/spapr_vscsi.c
> > > @@ -690,7 +690,7 @@ static void vscsi_inquiry_no_target(VSCSIState *s,
> > > vscsi_req *req)
> > >      int rc, len, alen;
> > >
> > >      /* We dont do EVPD. Also check that page_code is 0 */
> > > -    if ((cdb[1] & 0x01) || (cdb[1] & 0x01) || cdb[2] != 0) {
> > > +    if ((cdb[1] & 0x01) || cdb[2] != 0) {
> > >          /* Send INVALID FIELD IN CDB */
> > >          vscsi_makeup_sense(s, req, ILLEGAL_REQUEST, 0x24, 0);
> > >          vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
> >
> > Not trivial---I have no idea if something else was meant to be checked.
>
> Considering the code intends to follow the comment mentioned above if
> condition. I think the patch is correct.
>
> Here is reference from SCSI manual.
>
> <manual>
> INQUIRY Command
> ================
> EVPD (Enable Vital Product Data) bit
> ----------------------------------------------
> An enable vital product data (EVPD) bit set to one specifies that the
> device server shall return the vital product data specified by the
> PAGE CODE field (see 3.6.4).
>
> 0 ==> If the EVPD bit is set to zero, the device server shall return
> the standard INQUIRY data (see 3.6.2). If the PAGE CODE field is not
> set to zero when the EVPD bit is set to zero, the command shall be
> terminated with CHECK CONDITION status, with the sense key set to
> ILLEGAL REQUEST, and the additional sense code set to INVALID FIELD IN
> CDB.
>
> 1 ==> When the EVPD bit is set to one, the PAGE CODE field specifies
> which page of vital product data information the device server shall
> return (see 4.4).
> </manual>
>
> - According to comment EVPD is not supported. (First part of if condition)
> - According to manual if EVPD is 0, PAGE CODE must not be zero. PAGE
> CODE is 3rd byte of INQUIRY command (i.e. cmd[2])

More important, there's no other field in the CDB that matters (except 
for an obsolete on in byte 1/bit 1).

Applied to scsi-next branch, thanks.

Paolo