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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
