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

[Marcel Apfelbaum <marcel.apfelbaum@gmail.com>]

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."



  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 <clg@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;
>