spapr_vscsi: remove duplicate condition check
PATCH 1/1 spapr_vscsi: remove duplicate condition check
--- hw/scsi/spapr_vscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
hw/scsi/spapr_vscsi.c +1 -1
@@ -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 );
Signature · 1 line
1.8.1.2
Il 24/03/2014 16:44, Prasad Joshi ha scritto:
Quoted 11 lines
Signed-off-by Prasad Joshi <[email protected]>--- hw/scsi/spapr_vscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)hw/scsi/spapr_vscsi.c +0
-0
hw/scsi/spapr_vscsi.c static void vscsi_inquiry_no_target(VSCSIState *s, vscsi_req *req) · line 696
Quoted 8 lines
hw/scsi/spapr_vscsi.c +1
-1@@ -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
Quoted 15 lines · Paolo Bonzini
Il 24/03/2014 16:44, Prasad Joshi ha scritto:
Quoted 13 lines
Signed-off-by Prasad Joshi <[email protected]>--- hw/scsi/spapr_vscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)hw/scsi/spapr_vscsi.c +0
-0@@ -690,7 +690,7 @@ static void vscsi_inquiry_no_target(VSCSIState *s, vscsi_req *req)int rc, len, alen;
Quoted 9 lines
Quoted 7 lines
hw/scsi/spapr_vscsi.c +1
-1/* 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
Quoted 2 lines
Paolo
Paolo Bonzini
AppliedIl 24/03/2014 18:19, Prasad Joshi ha scritto:
Quoted 47 lines · Paolo Bonzini
Quoted 15 lines
Il 24/03/2014 16:44, Prasad Joshi ha scritto:
Quoted 13 lines
Signed-off-by Prasad Joshi <[email protected]>--- hw/scsi/spapr_vscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)hw/scsi/spapr_vscsi.c +0
-0@@ -690,7 +690,7 @@ static void vscsi_inquiry_no_target(VSCSIState *s, vscsi_req *req)int rc, len, alen;Quoted 9 lines
Quoted 7 lines
hw/scsi/spapr_vscsi.c +1
-1/* 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.
Quoted 8 lines
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