[Qemu-devel] [PATCH] spapr_vscsi: remove duplicate condition check

[Qemu-devel] [PATCH] spapr_vscsi: remove duplicate condition check

[Prasad Joshi]

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);
-- 
1.8.1.2

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

[Paolo Bonzini]

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.

Paolo

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

[Prasad Joshi]

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])

Therefore, in my opinion, patch is correct.

Thanks and Regards,
Prasad

>
> Paolo

Re: [Qemu-devel] [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 <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