Re: [PATCH] hw/scsi/megasas:Clean up some redundant code fix Clang warnings

[Laurent Vivier]

Le 10/03/2020 à 14:46, Peter Maydell a écrit :
> On Tue, 10 Mar 2020 at 13:10, Chen Qun <address@hidden> wrote:
>>
>> Here are some redundant statements, we can clean them up.
>> Clang static code analyzer show warning:
>> hw/scsi/megasas.c:1175:32: warning: Value stored to 'max_ld_disks' during 
>> its initialization is never read
>>     uint32_t num_ld_disks = 0, max_ld_disks = s->fw_luns;
>>                                ^~~~~~~~~~~~   ~~~~~~~~~~
>> hw/scsi/megasas.c:1183:9: warning: Value stored to 'max_ld_disks' is never 
>> read
>>         max_ld_disks = 0;
>>         ^              ~
>>
>> Reported-by: Euler Robot <address@hidden>
>> Signed-off-by: Chen Qun <address@hidden>
>> ---
>> Cc: Paolo Bonzini <address@hidden>
>> Cc: Fam Zheng <address@hidden>
>> Cc: Hannes Reinecke <address@hidden>
>> Cc: address@hidden
>> ---
>>  hw/scsi/megasas.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>> index af18c88b65..3f982e1d3b 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -1172,7 +1172,7 @@ static int megasas_dcmd_ld_list_query(MegasasState *s, 
>> MegasasCmd *cmd)
>>      uint16_t flags;
>>      struct mfi_ld_targetid_list info;
>>      size_t dcmd_size = sizeof(info), resid;
>> -    uint32_t num_ld_disks = 0, max_ld_disks = s->fw_luns;
>> +    uint32_t num_ld_disks = 0, max_ld_disks;
>>      BusChild *kid;
>>
>>      /* mbox0 contains flags */
>> @@ -1180,7 +1180,6 @@ static int megasas_dcmd_ld_list_query(MegasasState *s, 
>> MegasasCmd *cmd)
>>      trace_megasas_dcmd_ld_list_query(cmd->index, flags);
>>      if (flags != MR_LD_QUERY_TYPE_ALL &&
>>          flags != MR_LD_QUERY_TYPE_EXPOSED_TO_HOST) {
>> -        max_ld_disks = 0;
>>      }
> 
> This doesn't look right -- your change removes the only statement
> in the body of this "if". I think you need to examine what the
> function is trying to do with the test it is doing on these flags
> in order to identify what the right change is... Probably this
> means going back to the h/w spec to identify the correct behaviour
> overall.

Moreover this "if" is the only user of MR_LD_QUERY_TYPE_ALL and
MR_LD_QUERY_TYPE_EXPOSED_TO_HOST.

Thanks,
Laurent