| Message ID | 20180104024228.11780-1-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: Don't deference in_buf if NULL | expand |
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)?
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 ?
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
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>
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); }
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(-)