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 <c...@fr.ibm.com>
>> ---
>> 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;
>>
>
