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

[Hans de Goede <hdegoede@redhat.com>]

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<hdegoede@redhat.com>
>>>> ---
>>>> 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.

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.

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, and what should it do at close, do a set handlers NULL
before doing the actual close ??

Regards,

Hans