[Spice-devel] [PATCH v6 2/9] server: api: add spice_qxl_* calls based on QXLWorker contents

[Christophe Fergeau]

On Wed, Jul 20, 2011 at 12:10:07PM +0300, Alon Levy wrote:
> On Wed, Jul 20, 2011 at 10:53:49AM +0200, Christophe Fergeau wrote:
> > On Wed, Jul 20, 2011 at 11:19:52AM +0300, Alon Levy wrote:
> > > -static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> > > +static void red_dispatcher_update_area(RedDispatcher *dispatcher, uint32_t surface_id,
> > >                                     QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
> > >                                     uint32_t num_dirty_rects, uint32_t clear_dirty_region)
> > >  {
> > > -    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
> > >      RedWorkerMessage message = RED_WORKER_MESSAGE_UPDATE;
> > >      SpiceRect *dirty_rects = spice_new0(SpiceRect, num_dirty_rects);
> > >      SpiceRect *area = spice_new0(SpiceRect, 1);
> > > @@ -241,9 +240,16 @@ static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> > >      free(area);
> > >  }
> > >  
> > > -static void qxl_worker_add_memslot(QXLWorker *qxl_worker, QXLDevMemSlot *mem_slot)
> > > +static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> > > +                                   QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
> > > +                                   uint32_t num_dirty_rects, uint32_t clear_dirty_region)
> > > +{
> > > +    red_dispatcher_update_area((RedDispatcher*)qxl_worker, surface_id, qxl_area,
> > > +                               qxl_dirty_rects, num_dirty_rects, clear_dirty_region);
> > > +}
> > 
> > 
> > Aren't red_dispatcher_update_area/qxl_worker_update_area redundant? Can't
> > we just keep the old qxl_worker_update_area and do the cast in there?
> 
> red_disptacher_update_area is used twice - once by qxl_worker_update_area which must remain
> for backward compatibility (QXLWorker.update_area is set to it), and second by spice_qxl_update_area
> which goes directly from the QXLInstance to the RedDispatcher without going through QXLWorker.

Given the (RedDispatcher*)qxl_worker cast, in this case a RedDispatcher
will also be a QXLWorker and vice versa, so I don't understand why we need
a wrapper whose only added value is an obvious cast.

> > Will the function pointer above go away? My understanding is that the
> > functions added below are meant to replace these?
> 
> They can't go away as long as we want to maintain backwards compatibility.

Ok, I'd have gone with using these to implement spice_qxl_wakeup et al:
void spice_qxl_wakeup(QXLInstance *instance)
{
    QXLWorker *worker = (QXLWorker *)instance->st->dispatcher;
    ASSERT(worker->wakeup != NULL)
    worker->wakeup(worker);
}

or something like that

But maybe that's something kraxel wanted to avoid.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20110720/ee86d756/attachment-0001.pgp>