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

[Alon Levy]

On Wed, Jul 20, 2011 at 11:33:39AM +0200, Christophe Fergeau wrote:
> 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.
> 

I dunno, both would work. None look particularily pretty. Do you mind
if I don't do another rewrite?

> Christophe



> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel