From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=33241 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1Pruzd-00074k-VW
	for qemu-devel@nongnu.org; Tue, 22 Feb 2011 11:21:43 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <avi@redhat.com>) id 1Pruzc-00024i-QA
	for qemu-devel@nongnu.org; Tue, 22 Feb 2011 11:21:41 -0500
Received: from mx1.redhat.com ([209.132.183.28]:35063)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <avi@redhat.com>) id 1Pruzc-00024X-Ee
	for qemu-devel@nongnu.org; Tue, 22 Feb 2011 11:21:40 -0500
Message-ID: <4D63E299.8070808@redhat.com>
Date: Tue, 22 Feb 2011 18:21:45 +0200
From: Avi Kivity <avi@redhat.com>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH 2/7] iohandlers: Introduce a new API
References: <cover.1298369272.git.amit.shah@redhat.com>	<3d41a2ca603b8132e2496976e89fca09fc246683.1298369272.git.amit.shah@redhat.com>
	<4D63B9E2.5060201@redhat.com> <4D63CF82.8060006@codemonkey.ws>
In-Reply-To: <4D63CF82.8060006@codemonkey.ws>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Anthony Liguori <anthony@codemonkey.ws>
Cc: Amit Shah <amit.shah@redhat.com>, qemu list <qemu-devel@nongnu.org>, Gerd Hoffmann <kraxel@redhat.com>

On 02/22/2011 05:00 PM, Anthony Liguori wrote:
>>
>> Drop the opaque, instead put the IOHandler in there (or maybe the 
>> CharDev?) and use container_of().
>
>
> You know, I'm not there that this is automatically better.

Why not?  One less unsafe cast.

>
>>> +
>>> +int assign_iohandler(int fd, IOAllHandler *handler, unsigned int mask,
>>> +                     void *opaque);
>>> +int remove_iohandler(int fd);
>>> +int update_fd_mask(int fd, unsigned int mask);
>>> +int get_fd_mask(int fd, unsigned int *mask);
>>> +
>>
>> iohandler_init(IOHandler *ioh, int fd, IOEventHandler *handler, 
>> unsigned mask)
>> iohandler_del(IOHandler *ioh)
>> iohandler_set_event_mask(IOHandler *ioh, unsigned mask)
>> iohandler_event_mask(IOHandler *ioh)
>>
>> No opaques, use an object as a key so you don't have to search for it.
>
> I don't think there should be a setter/getter for the event mask.  
> Just delete the handler and re-add it.
>

It's more epoll() compatible to update it.  I'm fine with reregistering 
though.

> Why is there an IOHandler and an IOEventHandler argument for init?

IOHandler is the object, IOEventHandler is the function call.

Basically this is a 1:1 translation of

class IOHandler {
public:
     IOHandler(int fd, unsigned mask);
     ~IOHandler();
     virtual void io_event_handler() = 0;
     unsigned event_mask();
     void set_event_mask(unsigned mask);
}

class MyClient : private IOHandler {
     virtual void io_event_handler();
}

where the compiler automatically generates the container_of() to get to 
the actual MyClient pointer.