Re: [Qemu-arm] [PATCH v2 03/21] hw: timer: Add ASPEED RTC device

[Peter Maydell]

On Thu, 4 Jul 2019 at 08:49, Joel Stanley <address@hidden> wrote:
>
> On Tue, 2 Jul 2019 at 19:19, Peter Maydell <address@hidden> wrote:
> >
> > On Tue, 18 Jun 2019 at 17:53, Cédric Le Goater <address@hidden> wrote:
> > >
> > > From: Joel Stanley <address@hidden>
> > >
> > > The RTC is modeled to provide time and date functionality. It is
> > > initialised at zero to match the hardware.
> > >
> > > There is no modelling of the alarm functionality, which includes the IRQ
> > > line. As there is no guest code to exercise this function that is
> > > acceptable for now.
> > >
> > > Signed-off-by: Joel Stanley <address@hidden>
> > > Reviewed-by: Peter Maydell <address@hidden>
> >
> > Hi; Coverity complains about this function (CID 1402782):
> >
> > > +static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)
> > > +{
> > > +    struct tm tm;
> > > +    uint32_t year, cent;
> > > +    uint32_t reg1 = rtc->reg[COUNTER1];
> > > +    uint32_t reg2 = rtc->reg[COUNTER2];
> > > +
> > > +    tm.tm_mday = (reg1 >> 24) & 0x1f;
> > > +    tm.tm_hour = (reg1 >> 16) & 0x1f;
> > > +    tm.tm_min = (reg1 >> 8) & 0x3f;
> > > +    tm.tm_sec = (reg1 >> 0) & 0x3f;
> > > +
> > > +    cent = (reg2 >> 16) & 0x1f;
> > > +    year = (reg2 >> 8) & 0x7f;
> > > +    tm.tm_mon = ((reg2 >>  0) & 0x0f) - 1;
> > > +    tm.tm_year = year + (cent * 100) - 1900;
> > > +
> > > +    rtc->offset = qemu_timedate_diff(&tm);
> > > +}
> >
> > because the tm_wday field of 'struct tm tm' is not initialized
> > before we call qemu_timedate_diff(). This is a false
> > positive because the "read" of this field is just the place
> > in qemu_timedate_diff() that does "struct tm tmp = *tm;"
> > before calling mktime(), and mktime() ignores tm_wday.
> > We could make Coverity happy by using a struct initializer
> > on 'tm' here; on the other hand we don't do that anywhere else
> > which calls qemu_timedate_diff(), so maybe I should just mark
> > this a false positive?
>
> I don't have an opinion on which option to take. Perhaps mark it as a
> false positive?

Yeah, seems reasonable.

-- PMM