Re: [Qemu-devel] [PATCH 2/8] ipmi: use a function to initialize the SDR table

[Cédric Le Goater]

On 02/14/2016 09:31 AM, Marcel Apfelbaum wrote:
> On 02/09/2016 02:13 PM, Cédric Le Goater wrote:
>> This patch does not change anything.
> 
> Hi,
> 
> Well, it changes *something*, otherwise ... :)
> 
> Maybe "This is only a re-factoring."

Yes. This is much better. "does not change anything" doe not make sense ...

C.

> 
> 
> 
>  It only moves the code section
>> initializing the sdrs in its own routine and prepares ground for the
>> subsequent patches.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>   hw/ipmi/ipmi_bmc_sim.c | 49 
>> ++++++++++++++++++++++++++++---------------------
>>   1 file changed, 28 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>> index 13171336f7f1..0abc9cb5de94 100644
>> --- a/hw/ipmi/ipmi_bmc_sim.c
>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>> @@ -1703,6 +1703,33 @@ static const uint8_t init_sdrs[] = {
>>       0xff, 0xff, 0x00, 0x00, 0x00
>>   };
>>
>> +static void ipmi_sdr_init(IPMIBmcSim *ibs)
>> +{
>> +    unsigned int i;
>> +    unsigned int recid;
>> +
>> +    for (i = 0;;) {
> 
> 
> I am all for extracting the functionality in a function,
> what bothers me a little is the above construct.
> 
> Maybe you can find a more "standard: approach like:
> 
>  while (recid != 0xffff) {
>      ...
>  }
> 
> Or maybe is just me.
> 
> Thanks,
> Marcel
> 
>> +        struct ipmi_sdr_header *sdrh;
>> +        int len;
>> +        if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
>> +            error_report("Problem with recid 0x%4.4x", i);
>> +            return;
>> +        }
>> +        sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
>> +        len = ipmi_sdr_length(sdrh);
>> +        recid = ipmi_sdr_recid(sdrh);
>> +        if (recid == 0xffff) {
>> +            break;
>> +        }
>> +        if ((i + len) > sizeof(init_sdrs)) {
>> +            error_report("Problem with recid 0x%4.4x", i);
>> +            return;
>> +        }
>> +        sdr_add_entry(ibs, sdrh, len, NULL);
>> +        i += len;
>> +    }
>> +}
>> +
>>   static const VMStateDescription vmstate_ipmi_sim = {
>>       .name = TYPE_IPMI_BMC_SIMULATOR,
>>       .version_id = 1,
>> @@ -1735,7 +1762,6 @@ static void ipmi_sim_realize(DeviceState *dev, Error 
>> **errp)
>>   {
>>       IPMIBmc *b = IPMI_BMC(dev);
>>       unsigned int i;
>> -    unsigned int recid;
>>       IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
>>
>>       qemu_mutex_init(&ibs->lock);
>> @@ -1752,26 +1778,7 @@ static void ipmi_sim_realize(DeviceState *dev, Error 
>> **errp)
>>           ibs->sdr.last_clear[i] = 0xff;
>>       }
>>
>> -    for (i = 0;;) {
>> -        struct ipmi_sdr_header *sdrh;
>> -        int len;
>> -        if ((i + IPMI_SDR_HEADER_SIZE) > sizeof(init_sdrs)) {
>> -            error_report("Problem with recid 0x%4.4x", i);
>> -            return;
>> -        }
>> -        sdrh = (struct ipmi_sdr_header *) &init_sdrs[i];
>> -        len = ipmi_sdr_length(sdrh);
>> -        recid = ipmi_sdr_recid(sdrh);
>> -        if (recid == 0xffff) {
>> -            break;
>> -        }
>> -        if ((i + len) > sizeof(init_sdrs)) {
>> -            error_report("Problem with recid 0x%4.4x", i);
>> -            return;
>> -        }
>> -        sdr_add_entry(ibs, sdrh, len, NULL);
>> -        i += len;
>> -    }
>> +    ipmi_sdr_init(ibs);
>>
>>       ibs->acpi_power_state[0] = 0;
>>       ibs->acpi_power_state[1] = 0;
>>
>