[Qemu-devel] Re: [PATCH v2 0/5] iohandlers: Add support for enabling/disabling individual handlers

[Anthony Liguori <anthony@codemonkey.ws>]

On 01/17/2011 04:18 AM, Amit Shah wrote:
> On (Thu) Jan 13 2011 [13:17:22], Anthony Liguori wrote:
>    
>> On 01/13/2011 09:00 AM, Amit Shah wrote:
>>      
>>> Hi,
>>>
>>> This patchset adds new interfaces to work with iohandlers.  It adds:
>>>
>>> int assign_fd_handlers(int fd, IOHandlerOps *ops, void *opaque)
>>>     -- Specify io handlers for an fd
>>> int remove_fd_handlers(int fd)
>>>     -- Remove fd handlers for fd (mark ioh for deletion)
>>> int set_read_poll_fd_action(int fd, bool enable)
>>>     -- Enable or disable the fd_read_poll fd handler
>>> int set_read_fd_action(int fd, bool enable)
>>>     -- Enable or disable the fd_read fd handler
>>> int set_write_fd_action(int fd, bool enable)
>>>     -- Enable or disable the fd_read fd handler
>>>
>>> A new struct, IOHandlerOps, is added, to collect all the ops together
>>> instead of passing individual ones to functions.
>>>        
>> Instead of inventing new interfaces, I think we should steal^Wlearn
>> from established interfaces.
>>      
> Agreed.
>
> I do also think it'll be worthwhile pulling in one of the libraries to
> reduce the amount of qemu-specific code we have in the other cases as
> well.
>
>    
>>   Both libevent and glib have interfaces
>> that essentially boil down to:
>>
>> handle add_fd_event(loop, fd, ConditionMask, callback, opaque)
>> remove_event(loop, handle)
>>      
> This is quite similar to the Linux polling API.
>
> I don't know what the 'loop' parameter would do, though.
>    

It's to allow for multiple I/O threads.  There's one loop context per 
thread and you can register events specifically to a thread.

>> I think that's what we should move to.  All the stuff in our current
>> loop around allowing suppressing of read events is terrible as it
>> forces the main loop to poll.  That makes it impossible to use other
>> main loops because it's completely unusual.
>>      
> While setting new APIs possibly requires more discussion if we want to
> do it once and right, since it's also an internal API, we can keep
> evolving them.
>    

Yeah, but we should avoid unnecessary churn.  If we're going to do a 
major refactoring of an internal API, we should try to make the 
refactoring make sense as much as possible.

Eliminating the polling callbacks has been on the TODO for years now.  I 
don't think it make sense to refactor the API without at least doing this.

> So what's the accepted course of action now - take this new API, rework
> it to look like that of some other library's and then rework the
> main_loop, keep developing something in parallel till it satisfies
> everyone, etc.?
>
> Also -- this patchset was prompted by a bug in qemu chardevs that
> freezes guests if they write faster than the chardevs can consume.
> What should the strategy on fixing that bug be?
>    

Fix the loop API such that we're not just fixing one bug but that we can 
address a bunch of other bugs that are out there.

Regards,

Anthony Liguori

> 		Amit
>