Re: [Qemu-devel] [PATCH v3 7/7] hw/net/stellaris_enet: Convert to vmstate

[Peter Maydell <peter.maydell@linaro.org>]

On 6 May 2014 12:53, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> +{
>> +    stellaris_enet_state *s = opaque;
>> +    int i;
>> +
>> +    /* Sanitize inbound state. Note that next_packet is an index but
>> +     * np is a size; hence their valid upper bounds differ.
>> +     */
>> +    if (s->next_packet >= ARRAY_SIZE(s->rx)) {
>> +        return -1;
>> +    }
>> +
>> +    if (s->np > ARRAY_SIZE(s->rx)) {
>> +        return -1;
>> +    }
>> +
>> +    for (i = 0; i < ARRAY_SIZE(s->rx); i++) {
>> +        if (s->rx[i].len > ARRAY_SIZE(s->rx[i].data)) {
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    if (s->rx_fifo_offset > ARRAY_SIZE(s->rx[0].data) + 4) {
>> +        return -1;
>> +    }
>
> Can you explain that +4 ?
> I think I can see how it would end up equalling ARRAY_SIZE if
> you've just read the last 4 bytes, but how does it go beyond?

Whoops, I think this should be - 4, not + 4 (I think I
messed up when I rearranged this from "offset + 4 > ARRAY_SIZE"
to avoid the potential overflow in that expression.)
The DATA read code is going to read from the 4 bytes starting
at s->rx[s->next_packet].data + s->rx_fifo_offset, so
we need to make sure the offset doesn't allow that to
overrun. (When we read the last 4 bytes then
the rx_fifo_offset is reset to zero immediately, so at
migration it's never possible for it to be equal to
ARRAY_SIZE).

>> +
>> +    if (s->tx_fifo_len > ARRAY_SIZE(s->tx_fifo)) {
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_stellaris_enet = {
>> +    .name = "stellaris_enet",
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>> +    .minimum_version_id_old = 2,
>
> Weren't we killing off the minimum_version_id_old's ?

Yes, but we can't til the patch making it optional
hits master (it is in the current migration pullreq,
so if that goes in OK I'll just delete the _old line.)

thanks
-- PMM