Re: [Qemu-devel] [PATCH 1/3] throttle: add throttle_detach/attach_aio_context()

[Stefan Hajnoczi]

On Wed, May 14, 2014 at 05:05:58PM +0200, Benoît Canet wrote:
> The Wednesday 14 May 2014 à 16:22:45 (+0200), Stefan Hajnoczi wrote :
> > Block I/O throttling uses timers and currently always adds them to the
> > main loop.  Throttling will break if bdrv_set_aio_context() is used to
> > move a BlockDriverState to a different AioContext.
> > 
> > This patch adds throttle_detach/attach_aio_context() interfaces so the
> s/so the/to the/ ? 

Thanks, let me know how you feel about my replies below.  If there is no
other need to respin then let's let this commit description typo slide.

> > diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
> > index ab29b0b..b890613 100644
> > --- a/include/qemu/throttle.h
> > +++ b/include/qemu/throttle.h
> > @@ -67,6 +67,11 @@ typedef struct ThrottleState {
> 
> Can we make sure this header knows about AioContext in case
> there is another throttle user not block related ?

It already does because qemu-common.h includes qemu-typedefs.h.  We get
an opaque AioContext.

The point of the typedefs.h is to avoid pulling in all the headers when
callers just pass around pointers and don't need the actual definition.

In this case, I think the split makes sense because callers might pass
AioContext without actually using the AioContext API themselves.

> > diff --git a/tests/test-throttle.c b/tests/test-throttle.c
> > index 1d4ffd3..5fa5000 100644
> > --- a/tests/test-throttle.c
> > +++ b/tests/test-throttle.c
> > @@ -12,8 +12,10 @@
> >  
> >  #include <glib.h>
> >  #include <math.h>
> 
> > +#include "block/aio.h"
> 
> And remove this one eventually.

This include pulls in the AioContext API because the test case itself
manipulates the AioContext.