Re: [Qemu-devel] [PATCH 2/3] usb-redir: Call qemu_chr_guest_open/close

[Anthony Liguori]

On 08/08/2011 03:01 AM, Hans de Goede wrote:
> Hi,
>
> On 08/07/2011 11:30 PM, Anthony Liguori wrote:
> > On 08/07/2011 12:41 PM, Hans de Goede wrote:
> > > Hi,
> > >
> > > On 08/07/2011 05:52 PM, Anthony Liguori wrote:
> > > > On 08/07/2011 08:21 AM, Hans de Goede wrote:
> > > > > To let the chardev now we're ready start receiving data. This is
> > > > > necessary
> > > > > with the spicevmc chardev to get it registered with the spice-server.
> > > > >
> > > > > Signed-off-by: Hans de Goede<address@hidden>
> > > > > ---
> > > > > usb-redir.c | 3 +++
> > > > > 1 files changed, 3 insertions(+), 0 deletions(-)
> > > > >
> > > > > diff --git a/usb-redir.c b/usb-redir.c
> > > > > index e212993..ec88c0b 100644
> > > > > --- a/usb-redir.c
> > > > > +++ b/usb-redir.c
> > > > > @@ -809,6 +809,8 @@ static int usbredir_initfn(USBDevice *udev)
> > > > >
> > > > > qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read,
> > > > > usbredir_chardev_read, usbredir_chardev_event, dev);
> > > > > + /* Let the other side know we are ready */
> > > > > + qemu_chr_guest_open(dev->cs);
> > > >
> > > >
> > > > You should do guest_open before adding handlers.
> > >
> > > Erm, no, guest_open may lead to a callback in the
> > > chardev, to which it may respond by immediately queuing a few writes /
> > > doing a read.
> >
> > So after my char-flow changes, you won't be allowed to set handlers
> > unless you've called open.
>
> Why not do it the other way around? So don't allow open until the
> handlers are set. My reasoning
> behind this is that eventually we will want to have a struct describing
> a pipe endpoint, which
> will contain handlers (by then identical for both sides) and besides the
> struct a priv / user_data
> pointer which will get passed by the handlers when called.
>
> Then we will have a chardev_create or pipe_create call which will take a
> struct + user data ptr
> for both ends (so twice). This matches what currently our set handlers
> call does. But I would
> expect the open to come after the creation of the pipe.

BTW, I'm 90% of the way there in my queue:

http://repo.or.cz/w/qemu/aliguori.git/shortlog/refs/heads/char-flow

My plan is to have a CharPipe structure that has two CharDriverStates 
embedded in it.  The backend/frontends need to attach themselves to the 
CharDriverState.  I see that as open().

> At least to me it is much more logical to first set the handlers (which
> are really part
> of object creation) and then later do the open, this matches the common
> programming
> paradigm of having an init/create function and an open function.

But you need to change the handlers all of the time to implement flow 
control.  Today we overload the setting of handlers to have semantic 
meaning beyond setting the callbacks for various events.

The paradigm I think of is open()'ing a file, and then select()'ing on a 
file descriptor.

> Also forcing the set handlers after the open does not work well with
> virtio_console, as these
> are not open until the port inside the guest is opened. So then it would
> need to delay its
> set handlers till the first open,

Right, what's the problem with this?

 and what should it do at close, do a
> set handlers NULL
> before doing the actual close ??

No, close will automatically remove any added handlers.

Regards,

Anthony Liguori

> Regards,
>
> Hans