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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 24/03/2014 18:19, Prasad Joshi ha scritto:
> On Mon, Mar 24, 2014 at 9:15 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 24/03/2014 16:44, Prasad Joshi ha scritto:
>>
>>> Signed-off-by: Prasad Joshi <prasadjoshi.linux@gmail.com>
>>> ---
>>>  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