Re: [PATCH v2 3/5] hw/display/virtio-gpu-virgl: define callbacks in realize function

[Philippe Mathieu-Daudé]

On 30/4/23 23:48, Bernhard Beschow wrote:
> Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh 
> <gurchetansingh@chromium.org>:
> > From: Gurchetan Singh <gurchetansingh@chromium.org>
> >
> > This reduces the amount of renderer backend specific needed to
> > be exposed to the GL device.  We only need one realize function
> > per renderer backend.
> >
> > Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > v1: - Remove NULL inits (Philippe)
> >     - Use VIRTIO_GPU_BASE where possible (Philippe)
> > v2: - Fix unnecessary line break (Akihiko)
> >
> > hw/display/virtio-gpu-gl.c     | 15 ++++++---------
> > hw/display/virtio-gpu-virgl.c  | 35 ++++++++++++++++++++++++----------
> > include/hw/virtio/virtio-gpu.h |  7 -------
> > 3 files changed, 31 insertions(+), 26 deletions(-)


> > void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
> > {
> > -    VirtIOGPU *g = VIRTIO_GPU(qdev);
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> > +    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> > +
> > +    VirtIOGPUBase *bdev = VIRTIO_GPU_BASE(qdev);
> > +    VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_GET_CLASS(bdev);
> > +
> > +    VirtIOGPU *gpudev = VIRTIO_GPU(qdev);
> > +    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(gpudev);
> > +
> > +    vbc->gl_flushed = virtio_gpu_virgl_flushed;
> > +    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
> > +    vgc->process_cmd = virtio_gpu_virgl_process_cmd;
> > +    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
> > +
> > +    vdc->reset = virtio_gpu_virgl_reset;
>
> A realize method is supposed to modify a single instance only while we're 
> modifying the behavior of whole classes here, i.e. will affect every instance 
> of these classes. This goes against QOM design principles and will therefore be 
> confusing for people who are familiar with QOM in particular and OOP in 
> general. I think the code should be cleaned up in a different way if really 
> needed.

Doh I was too quick and totally missed this was an instance,
thanks for being careful Bernhard!