[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-devel] Re: [PATCH] fdc: Fix vmsave/restore regression

From: Juan Quintela
Subject: [Qemu-devel] Re: [PATCH] fdc: Fix vmsave/restore regression
Date: Thu, 12 Nov 2009 15:37:56 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux)

Jan Kiszka <address@hidden> wrote:
> Juan Quintela wrote:
>> Jan Kiszka <address@hidden> wrote:
>>> *** NOTE ***
>>> 'git shortlog|grep "reset + vmsd"' shows 10 such conversions. I only
>>> briefly checked the first one, and it looks similar broken. Could
>>> someone have a second look at them? Maybe it is also better to define a
>>> vmsd opaque in DeviceInfo, which would also allow to solve this issue
>>> differently.
>> I searched for .qdev.vmsd, and all the other uses are right as far as I
>> can see.
> Maybe it works, but it doesn't look clean to me.

It is how qdev works :p

> E.g. tcx.c,
> vmstate_tcx_post_load: it should be called with the DeviceState as
> opaque value, right? Then I'm missing container_of(d, TCXState,
> busdev.qdev).

typedef struct TCXState {
    SysBusDevice busdev;

struct SysBusDevice {
    DeviceState qdev;

As you can see, if you have a pointer to a TCXState, you also have a
pointer to a DeviceState (some for PCIDevice).
It needs to be the 1st value, tcx.c should really use DO_UPCAST() and
not container_of.  If the DeviceState is not the 1st field, qdev stops

int qdev_init(DeviceState *dev)
    qemu_register_reset(qdev_reset, dev);
    if (dev->info->vmsd)
        vmstate_register(-1, dev->info->vmsd, dev);

As you can see, if we are using qdev, what we need to check is that the
type of vmstate_foo is the same that the qdev type.

static const VMStateDescription vmstate_tcx = {
    .fields      = (VMStateField []) {
        VMSTATE_UINT16(height, TCXState),
Important bit here is TCXState

static SysBusDeviceInfo tcx_info = {
    .qdev.size  = sizeof(TCXState),
See that the value that we are creating is a TCXState, then things are

    .qdev.vmsd  = &vmstate_tcx,

qdev abuses void * to create OOP in C (vmstate does the same), there is
not a simple way to typecheck more this.  What we need is that the
functions that we put in the SysBusDeviceInfo in this case, all expect a
value of type TCXState in this case.  It is ok that they use a subset
from the start (SysBusDevice or DeviceState), but we can't do much more
than that.

What we do with reset:

static void tcx_reset(DeviceState *d)
    TCXState *s = container_of(d, TCXState, busdev.qdev);

is not different that

static void tcx_reset(void *opaque)
    TCXState *s = opaque;

And in the case of vmstate, we have to sent values that are not qdev
based yet, i.e. we can't use this trick.  We could de a
vmstate_qdev_register() with the other type, but will not help so much
(VMStateDescription has to still use void * inside).

Later, Juan.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]