Re: [Qemu-devel] [PULL 3/8] m68k: Add NeXTcube machine

[Thomas Huth <thuth@redhat.com>]

On 16/09/2019 12.48, Peter Maydell wrote:
> On Sat, 7 Sep 2019 at 16:47, Thomas Huth <huth@tuxfamily.org> wrote:
>>
>> It is still quite incomplete (no SCSI, no floppy emulation, no network,
>> etc.), but the firmware already shows up the debug monitor prompt in the
>> framebuffer display, so at least the very basics are already working.
>>
>> This code has been taken from Bryce Lanham's GSoC 2011 NeXT branch at
>>
>>  https://github.com/blanham/qemu-NeXT/blob/next-cube/hw/next-cube.c
>>
>> and altered quite a bit to fit the latest interface and coding conventions
>> of the current QEMU.
>>
>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Message-Id: <20190831074519.32613-4-huth@tuxfamily.org>
>> Signed-off-by: Thomas Huth <huth@tuxfamily.org
> 
> Hi; Coverity spotted an issue in this function
> (CID 1405664):
> 
> 
>> +static void nextscr2_write(NeXTState *s, uint32_t val, int size)
>> +{
>> +    static int led;
>> +    static int phase;
>> +    static uint8_t old_scr2;
>> +    static uint8_t rtc_command;
>> +    static uint8_t rtc_value;
>> +    static uint8_t rtc_status = 0x90;
>> +    static uint8_t rtc_return;
>> +    uint8_t scr2_2;
>> +
> 
> 
>> +                /* read the status 0x31 */
>> +                if (rtc_command == 0x31) {
>> +                    scr2_2 = scr2_2 & (~SCR2_RTDATA);
>> +                    /* for now 0x00 */
>> +                    if (0x00 & (0x80 >> (phase - 8))) {
> 
> 0 & anything can never be true, so the line below here is dead code.

Right. I'm going to have a closer look at this at the weekend, to see
what's the best way to fix it or whether it should simply be removed.

>> +                        scr2_2 |= SCR2_RTDATA;
>> +                    }
>> +                    rtc_return = (rtc_return << 1) |
>> +                                 ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
>> +                }
>> +
> 
> Incidentally, I see that this file has quite a lot of
> what seems to be essentially device emulation code in it
> (a bunch of IO MemoryRegions defined locally) -- ideally
> these could be split out into proper device objects at
> some point.

Yeah, it's all old code from 2011 ... I'll keep this in mind for future
clean-ups!

 Thanks,
  Thomas