From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46773) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUs5p-0002p8-6H for qemu-devel@nongnu.org; Sun, 14 Feb 2016 03:31:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aUs5k-0003in-3J for qemu-devel@nongnu.org; Sun, 14 Feb 2016 03:31:45 -0500 Received: from mail-wm0-x242.google.com ([2a00:1450:400c:c09::242]:35963) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUs5j-0003ij-PA for qemu-devel@nongnu.org; Sun, 14 Feb 2016 03:31:40 -0500 Received: by mail-wm0-x242.google.com with SMTP id a4so5359075wme.3 for ; Sun, 14 Feb 2016 00:31:39 -0800 (PST) References: <1455020010-17532-1-git-send-email-clg@fr.ibm.com> <1455020010-17532-3-git-send-email-clg@fr.ibm.com> From: Marcel Apfelbaum Message-ID: <56C03B68.3050408@gmail.com> Date: Sun, 14 Feb 2016 10:31:36 +0200 MIME-Version: 1.0 In-Reply-To: <1455020010-17532-3-git-send-email-clg@fr.ibm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 2/8] ipmi: use a function to initialize the SDR table Reply-To: marcel@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= , Corey Minyard Cc: "Michael S. Tsirkin" , qemu-devel@nongnu.org, Greg Kurz 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 > --- > 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; >