[Qemu-devel] [PATCH] scsi: Don't deference in_buf if NULL

[Qemu-devel] [PATCH] scsi: Don't deference in_buf if NULL

[Fam Zheng]

scsi_disk_emulate_command passes in_buf=NULL and in_len=0 in the
REQUEST_SENSE branch. Inline the fixed_in evaluation and put it after
the in_len test.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 scsi/utils.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/scsi/utils.c b/scsi/utils.c
index ddae650a99..9a0a925ef9 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -320,10 +320,8 @@ int scsi_convert_sense(uint8_t *in_buf, int in_len,
                        uint8_t *buf, int len, bool fixed)
 {
     SCSISense sense;
-    bool fixed_in;
 
-    fixed_in = (in_buf[0] & 2) == 0;
-    if (in_len && fixed == fixed_in) {
+    if (in_len && !!fixed == ((in_buf[0] & 2) == 0)) {
         memcpy(buf, in_buf, MIN(len, in_len));
         return MIN(len, in_len);
     }
-- 
2.14.3

Re: [Qemu-devel] [PATCH] scsi: Don't deference in_buf if NULL

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

On 01/03/2018 08:42 PM, Fam Zheng wrote:
> scsi_disk_emulate_command passes in_buf=NULL and in_len=0 in the
> REQUEST_SENSE branch. Inline the fixed_in evaluation and put it after
> the in_len test.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  scsi/utils.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/scsi/utils.c b/scsi/utils.c
> index ddae650a99..9a0a925ef9 100644
> --- a/scsi/utils.c
> +++ b/scsi/utils.c
> @@ -320,10 +320,8 @@ int scsi_convert_sense(uint8_t *in_buf, int in_len,
>                         uint8_t *buf, int len, bool fixed)
>  {
>      SCSISense sense;
> -    bool fixed_in;
>  
> -    fixed_in = (in_buf[0] & 2) == 0;
> -    if (in_len && fixed == fixed_in) {
> +    if (in_len && !!fixed == ((in_buf[0] & 2) == 0)) {

Huh?  'fixed' is already a bool, so '!!fixed' is just extra typing that
makes things harder to read.  Did you mean:

if (in_len && fixed == !(in_buf[0] & 2))

as something that is slightly more legible (the LHS is already bool, the
RHS uses a single ! to convert a bitwise test into a bool with the
correct sense)?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH] scsi: Don't deference in_buf if NULL

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

On 01/03/2018 08:42 PM, Fam Zheng wrote:
> scsi_disk_emulate_command passes in_buf=NULL and in_len=0 in the
> REQUEST_SENSE branch. Inline the fixed_in evaluation and put it after
> the in_len test.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  scsi/utils.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Does this patch duplicate
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg04964.html ?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH] scsi: Don't deference in_buf if NULL

[Fam Zheng]

On Thu, 01/04 11:41, Eric Blake wrote:
> On 01/03/2018 08:42 PM, Fam Zheng wrote:
> > scsi_disk_emulate_command passes in_buf=NULL and in_len=0 in the
> > REQUEST_SENSE branch. Inline the fixed_in evaluation and put it after
> > the in_len test.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  scsi/utils.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> Does this patch duplicate
> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg04964.html ?

Yes. Thanks for pointing out this.

Fam

Re: [Qemu-devel] [PATCH] scsi: Don't deference in_buf if NULL

[Anthoine Bourgeois]

On Thu, Jan 04, 2018 at 11:02:25AM -0600, Eric Blake wrote:
>On 01/03/2018 08:42 PM, Fam Zheng wrote:
>> scsi_disk_emulate_command passes in_buf=NULL and in_len=0 in the
>> REQUEST_SENSE branch. Inline the fixed_in evaluation and put it after
>> the in_len test.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>  scsi/utils.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>
>Huh?  'fixed' is already a bool, so '!!fixed' is just extra typing that
>makes things harder to read.  Did you mean:
>
>if (in_len && fixed == !(in_buf[0] & 2))
>
>as something that is slightly more legible (the LHS is already bool, the
>RHS uses a single ! to convert a bitwise test into a bool with the
>correct sense)?

It seems correct and clearer.

With Eric's modification:
Tested-by: Anthoine Bourgeois <anthoine.bourgeois@gmail.com>