| Message ID | 1312723284-7549-2-git-send-email-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. Regards, Anthony Liguori > > return 0; > } > @@ -830,6 +832,7 @@ static void usbredir_handle_destroy(USBDevice *udev) > { > USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev); > > + qemu_chr_guest_close(dev->cs); > qemu_chr_close(dev->cs); > /* Note must be done after qemu_chr_close, as that causes a close event */ > qemu_bh_delete(dev->open_close_bh);
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. To me it makes much more sense to actually call guest_open when we are ready to receive data / to be read from, rather then to do it before our handlers are hooked up and thus before we are ready. Regards, Hans
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. We want qemu_chr_guest_open() -> qemu_chr_fe_open() and for it to be analogous to a qemu_chr_be_open() which would be called immediately after accept() returned on a socket to signal that the backend is opened. Because there's an intermediate queue, even if a write happens after open, no data will be lost. So conceptionally, it makes sense to set handlers after open IMHO. But most importantly to this series, no backend can possibly generate a write before you get to call add handlers so you've got nothing to worry about here (based on the code today). Regards, Anthony Liguori To me it makes much more sense to actually call guest_open > when we are ready to receive data / to be read from, rather then to do > it before our handlers are hooked up and thus before we are ready. > > Regards, > > Hans >
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
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<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. 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 >
Hi, On 08/08/2011 02:52 PM, Anthony Liguori wrote: > On 08/08/2011 03:01 AM, Hans de Goede wrote: <snip> >>> 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(). > So the attaching will cause the other end to see an open() (if the other end is already attached) ? Or will their still be a separate send open event call? And should that call be made before or after the attach? Regards, Hans
On 08/08/2011 08:03 AM, Hans de Goede wrote: > Hi, > > On 08/08/2011 02:52 PM, Anthony Liguori wrote: >> On 08/08/2011 03:01 AM, Hans de Goede wrote: > > <snip> > >>>> 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(). >> > > So the attaching will cause the other end to see an open() (if the > other end is already attached) ? Or will their still be a separate > send open event call? And should that call be made before or after > the attach? Doing an open() will result in an open event being generated. Neither side should ever be directly involved in sending open/close events. Regards, Anthony Liguori > > Regards, > > Hans >
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); return 0; } @@ -830,6 +832,7 @@ static void usbredir_handle_destroy(USBDevice *udev) { USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev); + qemu_chr_guest_close(dev->cs); qemu_chr_close(dev->cs); /* Note must be done after qemu_chr_close, as that causes a close event */ qemu_bh_delete(dev->open_close_bh);
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(-)