Re: [Qemu-devel] [PATCH 1/8] ipmi: fix SDR length value

[Corey Minyard]

On 01/06/2016 02:14 AM, Cédric Le Goater wrote:
> On 01/05/2016 08:59 PM, Eric Blake wrote:
> > On 01/05/2016 10:29 AM, Cédric Le Goater wrote:
> >
> > [meta-comment] Your messages were not marked in-reply-to: the 0/8 cover
> > letter, but came through as separate threads.  This makes it harder to
> > follow, especially in mail clients that sort top-level threads by most
> > recent activity on the thread.
> Yes. My bad. I put 'thread = false' in my .gitconfig for some reason and
> didn't check before sending. This is fixed.
>
> > > The IPMI BMC simulator populates the SDR table with a set of initial
> > > SDRs. The length of each SDR is taken from the record itself (byte 4)
> > > which does not include the size of the header. But, the full length
> > > (header + data) is required by the sdr_add_entry() routine.
> > >
> > > Signed-off-by: Cédric Le Goater <address@hidden>
> > > ---
> > >
> > >   Maybe we could use a sdr struct/typedef to clarify the code. See
> > >   patch 7: "ipmi: introduce an ipmi_bmc_init_sensor() API"
> > >
> > >   hw/ipmi/ipmi_bmc_sim.c | 14 +++++++-------
> > >   1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> > > index 0a59e539f549..559e1398d669 100644
> > > --- a/hw/ipmi/ipmi_bmc_sim.c
> > > +++ b/hw/ipmi/ipmi_bmc_sim.c
> > > @@ -362,7 +362,7 @@ static int sdr_find_entry(IPMISdr *sdr, uint16_t recid,
> > >   
> > >       while (pos < sdr->next_free) {
> > >           uint16_t trec = sdr->sdr[pos] | (sdr->sdr[pos + 1] << 8);
> > > -        unsigned int nextpos = pos + sdr->sdr[pos + 4];
> > > +        unsigned int nextpos = pos + sdr->sdr[pos + 4] + 5;
> > 5 feels like a magic number; should you use a #define and name it?
> Yes. 5 being the sdr header length.
>
> The simulator uses a lot of these byte offsets and I think the code
> would gain to use a struct as proposed in patch 7:
>    
>    "ipmi: introduce an ipmi_bmc_init_sensor() API".
>
> Corey, is there a reason for not doing so ?

I was just adding one and it didn't matter much at that point?  Or I was 
lazy?

I've commented a little earlier on patch 7, the struct is a better way 
to go.

-corey

> > > @@ -1709,20 +1709,20 @@ static void ipmi_sim_init(Object *obj)
> > >       for (i = 0;;) {
> > >           int len;
> > >           if ((i + 5) > sizeof(init_sdrs)) {
> > > -            error_report("Problem with recid 0x%4.4x: \n", i);
> > > +            error_report("Problem with recid 0x%4.4x\n", i);
> > Please drop the trailing \n as long as you are touching this.
> Sure.
>
> Thanks,
>
> C.
>