Patchwork [1/2] char: add qemu_chr_be_is_fe_connected

login
register
mail settings
Submitter Alon Levy
Date March 21, 2013, 4:35 p.m.
Message ID <1363883716-30289-2-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/229762/
State New
Headers show

Comments

Alon Levy - March 21, 2013, 4:35 p.m.
Note that the handler is called chr_is_guest_connected and not
chr_is_fe_connected, consistent with other members of CharDriverState.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/virtio-console.c |  9 +++++++++
 include/char/char.h | 11 +++++++++++
 qemu-char.c         |  9 +++++++++
 3 files changed, 29 insertions(+)
Anthony Liguori - March 21, 2013, 6:18 p.m.
Alon Levy <alevy@redhat.com> writes:

> Note that the handler is called chr_is_guest_connected and not
> chr_is_fe_connected, consistent with other members of CharDriverState.

Sorry, I don't get it.

There isn't a notion of "connected" for the front-ends in the char
layer.  The closest thing is whether add_handlers() have been called or
not.

I really dislike the idea of introduction a new concept to the char
layer in a half baked way.

Why can't migration notifiers be used for this?  I think Gerd objected
to using a migration *handler* but not necessarily a state notifier.

Regards,

Anthony Liguori

>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  hw/virtio-console.c |  9 +++++++++
>  include/char/char.h | 11 +++++++++++
>  qemu-char.c         |  9 +++++++++
>  3 files changed, 29 insertions(+)
>
> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> index e2d1c58..643e24e 100644
> --- a/hw/virtio-console.c
> +++ b/hw/virtio-console.c
> @@ -120,6 +120,13 @@ static void chr_event(void *opaque, int event)
>      }
>  }
>  
> +static bool chr_is_guest_connected(void *opaque)
> +{
> +    VirtConsole *vcon = opaque;
> +
> +    return vcon->port.guest_connected;
> +}
> +
>  static int virtconsole_initfn(VirtIOSerialPort *port)
>  {
>      VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
> @@ -133,6 +140,8 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
>      if (vcon->chr) {
>          qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
>                                vcon);
> +        /* only user of chr_is_guest_connected so leave it as special cased*/
> +        vcon->chr->chr_is_guest_connected = chr_is_guest_connected;
>      }
>  
>      return 0;
> diff --git a/include/char/char.h b/include/char/char.h
> index 0326b2a..b41ddc0 100644
> --- a/include/char/char.h
> +++ b/include/char/char.h
> @@ -52,6 +52,7 @@ typedef struct {
>  #define CHR_TIOCM_RTS	0x004
>  
>  typedef void IOEventHandler(void *opaque, int event);
> +typedef bool IOIsGuestConnectedHandler(void *opaque);
>  
>  struct CharDriverState {
>      void (*init)(struct CharDriverState *s);
> @@ -64,6 +65,7 @@ struct CharDriverState {
>      IOEventHandler *chr_event;
>      IOCanReadHandler *chr_can_read;
>      IOReadHandler *chr_read;
> +    IOIsGuestConnectedHandler *chr_is_guest_connected;
>      void *handler_opaque;
>      void (*chr_close)(struct CharDriverState *chr);
>      void (*chr_accept_input)(struct CharDriverState *chr);
> @@ -229,6 +231,15 @@ void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len);
>   */
>  void qemu_chr_be_event(CharDriverState *s, int event);
>  
> +/**
> + * @qemu_chr_be_is_fe_connected:
> + *
> + * Back end calls this to check if the front end is connected.
> + *
> + * Returns: true if the guest (front end) is connected, false otherwise.
> + */
> +bool qemu_chr_be_is_fe_connected(CharDriverState *s);
> +
>  void qemu_chr_add_handlers(CharDriverState *s,
>                             IOCanReadHandler *fd_can_read,
>                             IOReadHandler *fd_read,
> diff --git a/qemu-char.c b/qemu-char.c
> index 4e011df..77a501a 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -120,6 +120,15 @@ void qemu_chr_be_event(CharDriverState *s, int event)
>      s->chr_event(s->handler_opaque, event);
>  }
>  
> +bool qemu_chr_be_is_fe_connected(CharDriverState *s)
> +{
> +    if (s->chr_is_guest_connected) {
> +        return s->chr_is_guest_connected(s->handler_opaque);
> +    }
> +    /* default to always connected */
> +    return true;
> +}
> +
>  static gboolean qemu_chr_generic_open_bh(gpointer opaque)
>  {
>      CharDriverState *s = opaque;
> -- 
> 1.8.1.4
Alon Levy - March 21, 2013, 6:35 p.m.
> Alon Levy <alevy@redhat.com> writes:
> 
> > Note that the handler is called chr_is_guest_connected and not
> > chr_is_fe_connected, consistent with other members of
> > CharDriverState.
> 
> Sorry, I don't get it.
> 
> There isn't a notion of "connected" for the front-ends in the char
> layer.  The closest thing is whether add_handlers() have been called
> or
> not.

It makes sense for virtio-console - it matches exactly the internal guest_connected port state.

> 
> I really dislike the idea of introduction a new concept to the char
> layer in a half baked way.

Is the fact there is only one user, virtio-console, the reason you call this half baked?

> 
> Why can't migration notifiers be used for this?  I think Gerd
> objected
> to using a migration *handler* but not necessarily a state notifier.

Because if you have two chardevices, i.e.
 -chardev spicevmc,name=vdagent,id=spice1 -chardev spicevmc,name=vdagent,id=spice2

Then the two on-the-wire vmstates will be identical.

> 
> Regards,
> 
> Anthony Liguori
> 
> >
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> >  hw/virtio-console.c |  9 +++++++++
> >  include/char/char.h | 11 +++++++++++
> >  qemu-char.c         |  9 +++++++++
> >  3 files changed, 29 insertions(+)
> >
> > diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> > index e2d1c58..643e24e 100644
> > --- a/hw/virtio-console.c
> > +++ b/hw/virtio-console.c
> > @@ -120,6 +120,13 @@ static void chr_event(void *opaque, int event)
> >      }
> >  }
> >  
> > +static bool chr_is_guest_connected(void *opaque)
> > +{
> > +    VirtConsole *vcon = opaque;
> > +
> > +    return vcon->port.guest_connected;
> > +}
> > +
> >  static int virtconsole_initfn(VirtIOSerialPort *port)
> >  {
> >      VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
> > @@ -133,6 +140,8 @@ static int virtconsole_initfn(VirtIOSerialPort
> > *port)
> >      if (vcon->chr) {
> >          qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read,
> >          chr_event,
> >                                vcon);
> > +        /* only user of chr_is_guest_connected so leave it as
> > special cased*/
> > +        vcon->chr->chr_is_guest_connected =
> > chr_is_guest_connected;
> >      }
> >  
> >      return 0;
> > diff --git a/include/char/char.h b/include/char/char.h
> > index 0326b2a..b41ddc0 100644
> > --- a/include/char/char.h
> > +++ b/include/char/char.h
> > @@ -52,6 +52,7 @@ typedef struct {
> >  #define CHR_TIOCM_RTS	0x004
> >  
> >  typedef void IOEventHandler(void *opaque, int event);
> > +typedef bool IOIsGuestConnectedHandler(void *opaque);
> >  
> >  struct CharDriverState {
> >      void (*init)(struct CharDriverState *s);
> > @@ -64,6 +65,7 @@ struct CharDriverState {
> >      IOEventHandler *chr_event;
> >      IOCanReadHandler *chr_can_read;
> >      IOReadHandler *chr_read;
> > +    IOIsGuestConnectedHandler *chr_is_guest_connected;
> >      void *handler_opaque;
> >      void (*chr_close)(struct CharDriverState *chr);
> >      void (*chr_accept_input)(struct CharDriverState *chr);
> > @@ -229,6 +231,15 @@ void qemu_chr_be_write(CharDriverState *s,
> > uint8_t *buf, int len);
> >   */
> >  void qemu_chr_be_event(CharDriverState *s, int event);
> >  
> > +/**
> > + * @qemu_chr_be_is_fe_connected:
> > + *
> > + * Back end calls this to check if the front end is connected.
> > + *
> > + * Returns: true if the guest (front end) is connected, false
> > otherwise.
> > + */
> > +bool qemu_chr_be_is_fe_connected(CharDriverState *s);
> > +
> >  void qemu_chr_add_handlers(CharDriverState *s,
> >                             IOCanReadHandler *fd_can_read,
> >                             IOReadHandler *fd_read,
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 4e011df..77a501a 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -120,6 +120,15 @@ void qemu_chr_be_event(CharDriverState *s, int
> > event)
> >      s->chr_event(s->handler_opaque, event);
> >  }
> >  
> > +bool qemu_chr_be_is_fe_connected(CharDriverState *s)
> > +{
> > +    if (s->chr_is_guest_connected) {
> > +        return s->chr_is_guest_connected(s->handler_opaque);
> > +    }
> > +    /* default to always connected */
> > +    return true;
> > +}
> > +
> >  static gboolean qemu_chr_generic_open_bh(gpointer opaque)
> >  {
> >      CharDriverState *s = opaque;
> > --
> > 1.8.1.4
> 
> 
>
Anthony Liguori - March 21, 2013, 7:24 p.m.
Alon Levy <alevy@redhat.com> writes:

>> Alon Levy <alevy@redhat.com> writes:
>> 
>> > Note that the handler is called chr_is_guest_connected and not
>> > chr_is_fe_connected, consistent with other members of
>> > CharDriverState.
>> 
>> Sorry, I don't get it.
>> 
>> There isn't a notion of "connected" for the front-ends in the char
>> layer.  The closest thing is whether add_handlers() have been called
>> or
>> not.
>
> It makes sense for virtio-console - it matches exactly the internal
> guest_connected port state.

I still don't understand why you need to know that detail in the
backend.  Hint: you should explain this in future commit messages/cover
letters.

>> I really dislike the idea of introduction a new concept to the char
>> layer in a half baked way.
>
> Is the fact there is only one user, virtio-console, the reason you
> call this half baked?

You are introducing a function:

qemu_chr_be_is_virtio_console_connnected()

And calling pretending like it's a generic character device interface.
It's not.

If spicevmc only works with virtio-console and has no hope of working
with anything else, don't use the character device layer!  It's trying
to fit a square peg into a round hole.

If it's a concept that makes sense for all character devices front ends,
then it should be a patch making it be a meaningful to all character
device front end implementations.

>> Why can't migration notifiers be used for this?  I think Gerd
>> objected
>> to using a migration *handler* but not necessarily a state notifier.
>
> Because if you have two chardevices, i.e.
>  -chardev spicevmc,name=vdagent,id=spice1 -chardev spicevmc,name=vdagent,id=spice2
>
> Then the two on-the-wire vmstates will be identical.

I don't understand why this matters but I don't understand what the
problem you're trying to solve is either so that's not surprising.

Regards,

Anthony Liguori

>
>> 
>> Regards,
>> 
>> Anthony Liguori
>> 
>> >
>> > Signed-off-by: Alon Levy <alevy@redhat.com>
>> > ---
>> >  hw/virtio-console.c |  9 +++++++++
>> >  include/char/char.h | 11 +++++++++++
>> >  qemu-char.c         |  9 +++++++++
>> >  3 files changed, 29 insertions(+)
>> >
>> > diff --git a/hw/virtio-console.c b/hw/virtio-console.c
>> > index e2d1c58..643e24e 100644
>> > --- a/hw/virtio-console.c
>> > +++ b/hw/virtio-console.c
>> > @@ -120,6 +120,13 @@ static void chr_event(void *opaque, int event)
>> >      }
>> >  }
>> >  
>> > +static bool chr_is_guest_connected(void *opaque)
>> > +{
>> > +    VirtConsole *vcon = opaque;
>> > +
>> > +    return vcon->port.guest_connected;
>> > +}
>> > +
>> >  static int virtconsole_initfn(VirtIOSerialPort *port)
>> >  {
>> >      VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
>> > @@ -133,6 +140,8 @@ static int virtconsole_initfn(VirtIOSerialPort
>> > *port)
>> >      if (vcon->chr) {
>> >          qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read,
>> >          chr_event,
>> >                                vcon);
>> > +        /* only user of chr_is_guest_connected so leave it as
>> > special cased*/
>> > +        vcon->chr->chr_is_guest_connected =
>> > chr_is_guest_connected;
>> >      }
>> >  
>> >      return 0;
>> > diff --git a/include/char/char.h b/include/char/char.h
>> > index 0326b2a..b41ddc0 100644
>> > --- a/include/char/char.h
>> > +++ b/include/char/char.h
>> > @@ -52,6 +52,7 @@ typedef struct {
>> >  #define CHR_TIOCM_RTS	0x004
>> >  
>> >  typedef void IOEventHandler(void *opaque, int event);
>> > +typedef bool IOIsGuestConnectedHandler(void *opaque);
>> >  
>> >  struct CharDriverState {
>> >      void (*init)(struct CharDriverState *s);
>> > @@ -64,6 +65,7 @@ struct CharDriverState {
>> >      IOEventHandler *chr_event;
>> >      IOCanReadHandler *chr_can_read;
>> >      IOReadHandler *chr_read;
>> > +    IOIsGuestConnectedHandler *chr_is_guest_connected;
>> >      void *handler_opaque;
>> >      void (*chr_close)(struct CharDriverState *chr);
>> >      void (*chr_accept_input)(struct CharDriverState *chr);
>> > @@ -229,6 +231,15 @@ void qemu_chr_be_write(CharDriverState *s,
>> > uint8_t *buf, int len);
>> >   */
>> >  void qemu_chr_be_event(CharDriverState *s, int event);
>> >  
>> > +/**
>> > + * @qemu_chr_be_is_fe_connected:
>> > + *
>> > + * Back end calls this to check if the front end is connected.
>> > + *
>> > + * Returns: true if the guest (front end) is connected, false
>> > otherwise.
>> > + */
>> > +bool qemu_chr_be_is_fe_connected(CharDriverState *s);
>> > +
>> >  void qemu_chr_add_handlers(CharDriverState *s,
>> >                             IOCanReadHandler *fd_can_read,
>> >                             IOReadHandler *fd_read,
>> > diff --git a/qemu-char.c b/qemu-char.c
>> > index 4e011df..77a501a 100644
>> > --- a/qemu-char.c
>> > +++ b/qemu-char.c
>> > @@ -120,6 +120,15 @@ void qemu_chr_be_event(CharDriverState *s, int
>> > event)
>> >      s->chr_event(s->handler_opaque, event);
>> >  }
>> >  
>> > +bool qemu_chr_be_is_fe_connected(CharDriverState *s)
>> > +{
>> > +    if (s->chr_is_guest_connected) {
>> > +        return s->chr_is_guest_connected(s->handler_opaque);
>> > +    }
>> > +    /* default to always connected */
>> > +    return true;
>> > +}
>> > +
>> >  static gboolean qemu_chr_generic_open_bh(gpointer opaque)
>> >  {
>> >      CharDriverState *s = opaque;
>> > --
>> > 1.8.1.4
>> 
>> 
>>
Hans de Goede - March 22, 2013, 7:56 a.m.
Hi,

On 03/21/2013 07:18 PM, Anthony Liguori wrote:
> Alon Levy <alevy@redhat.com> writes:
>
>> Note that the handler is called chr_is_guest_connected and not
>> chr_is_fe_connected, consistent with other members of CharDriverState.
>
> Sorry, I don't get it.
>
> There isn't a notion of "connected" for the front-ends in the char
> layer.

And that is simply completely and utterly wrong. We've tried to explain
this to you a number of times already. Yet you claim we've not explained
it. Or we've not explained it properly. I must say however that I'm
getting the feeling the problem is not us not explaining, but that you
are not listening.

Still let me try to explain it 1 more time, in 2 different ways:

1) At an abstract level a chardev fe + be is a pipe between somewhere
and where-ever. Both ends of the pipe can be opened or closed, not just
one.

Frontends end inside the guest usually in the form of some piece of
virtual hardware inside the guest. Just because the virtual hardware
is there does not mean the guest has a driver, if the guest has
a driver that creates a device-node for it, that does not mean there
is a userspace process inside the guest which actually has the
device-node open. So you see the front-end device-node inside the guest
can be opened and closed. Most frontends cannot signal this to the
backend but virtio-console can, and we want to know about it since
we want to know if the user-space agent is there.

2) At the code level it clearly is there too, backend open-close
is signalled to the frontend by means of the backend calling
qemu_chr_be_event with an event of CHR_EVENT_OPENED and
CHR_EVENT_CLOSED. Frontend open-close is signalled by the
frontend calling qemu_chr_fe_open and qemu_chr_fe_close.

Now the problem we have is that after migration the CHR_EVENT_OPENED
gets replayed on the destination side but the qemu_chr_fe_open call
does not.

The logical place to replay the qemu_chr_fe_open would be in the
frontend's migration handling code, as suggested here:
http://lists.gnu.org/archive/html/qemu-devel/2012-11/msg02721.html

But Amit did not like this approach, suggesting that instead we
took care of this inside spice-qemu-char.c. Which is what we're
trying to do now, but this requires spice-qemu-char.c being
able to query the open state of the frontend, which is already
being migrate by the virtio-console code in the form of the
guest_connected variable, we just need to be able to get to that
variable from the backend.

Regards,

Hans
Gerd Hoffmann - March 22, 2013, 8:25 a.m.
Hi,

> There isn't a notion of "connected" for the front-ends in the char
> layer.  The closest thing is whether add_handlers() have been called or
> not.

It isn't new.  There are qemu_chr_fe_open + qemu_chr_fe_close doing
exactly that signaling (whenever someone has opened the virtio-serial
port, i.e. whenever the guest agent is active or not).

Problem is the chardev forgets this state over migration.

So we need a way to restore the state.  virtio-serial knows it of course
as this is guest state it has to keep track of it.  We just need a way
the chardev can figure what the current state is after migration.

The options I see:

  (1) Have chardev store the state itself in a new migration section.
      This is what I've NAK'ed.  First, because live-migration host
      state is a can of worms I don't want open.  Second because it
      actually isn't host state.

  (2) virtio-serial could just call qemu_chr_fe_open in
      post_migration hook.  Could have unwanted side effects as
      the chardev can't figure whenever this is a post-load call
      or a guest-open call.

  (3) Add a new qemu_chr_fe_* call for virtio-serial to notify the
      chardev.  This was the next proposal from Alon

  (4) Do it the other way around:  Allow the chardev query what the
      current state is.  This patch series.

  (5) Cut off the chardev layer altogether and implement spicevmc as
      virtio-serial port.  Your proposal if I understand it correctly.
      Makes sense, but breaks backward compatibility, so even when
      doing this we must fix the chardev spicevmc bug somehow.

Hope this clarifies,
  Gerd
Hans de Goede - March 22, 2013, 8:58 a.m.
Hi,

On 03/22/2013 09:25 AM, Gerd Hoffmann wrote:
>    Hi,
>
>> There isn't a notion of "connected" for the front-ends in the char
>> layer.  The closest thing is whether add_handlers() have been called or
>> not.
>
> It isn't new.  There are qemu_chr_fe_open + qemu_chr_fe_close doing
> exactly that signaling (whenever someone has opened the virtio-serial
> port, i.e. whenever the guest agent is active or not).
>
> Problem is the chardev forgets this state over migration.
>
> So we need a way to restore the state.  virtio-serial knows it of course
> as this is guest state it has to keep track of it.  We just need a way
> the chardev can figure what the current state is after migration.
>
> The options I see:
>
>    (1) Have chardev store the state itself in a new migration section.
>        This is what I've NAK'ed.  First, because live-migration host
>        state is a can of worms I don't want open.  Second because it
>        actually isn't host state.
>
>    (2) virtio-serial could just call qemu_chr_fe_open in
>        post_migration hook.  Could have unwanted side effects as
>        the chardev can't figure whenever this is a post-load call
>        or a guest-open call.
>
>    (3) Add a new qemu_chr_fe_* call for virtio-serial to notify the
>        chardev.  This was the next proposal from Alon
>
>    (4) Do it the other way around:  Allow the chardev query what the
>        current state is.  This patch series.
>

Thanks for the above overview, very useful!

>    (5) Cut off the chardev layer altogether and implement spicevmc as
>        virtio-serial port.  Your proposal if I understand it correctly.
>        Makes sense, but breaks backward compatibility, so even when
>        doing this we must fix the chardev spicevmc bug somehow.

I've to nack this one, spicevmc is a chardev backend which is also used
with other frontends then just virtio-console, atm it is also used together
with the usb-redir and smartcard frontends and there is no reason why it
could not be used with others in the future, so: NACK.

Regards,

Hans
Anthony Liguori - March 22, 2013, 1:33 p.m.
Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> There isn't a notion of "connected" for the front-ends in the char
>> layer.  The closest thing is whether add_handlers() have been called or
>> not.
>
> It isn't new.  There are qemu_chr_fe_open + qemu_chr_fe_close doing
> exactly that signaling (whenever someone has opened the virtio-serial
> port, i.e. whenever the guest agent is active or not).

anthony@titi:~/git/qemu$ find -name "*.[ch]" -exec grep -l qemu_chr_fe_open {} \;
./qemu-char.c
./hw/virtio-console.c
./hw/usb/redirect.c
./include/char/char.h

And:

anthony@titi:~/git/qemu$ find . -name "*.[ch]" -exec grep -l chr_guest_open  {} \;
./qemu-char.c
./include/char/char.h
./spice-qemu-char.c

So this whole interface exists specificially for the spice-qemu-char
backend and only for that backend.  This isn't a generic char layer
concept.  It's a thinly veiled call into spice.

Now spice appears to have a work around in it where the first write will
trigger affectively the same action as doing an open.  Presumably this
makes it usable with !virtio-serial or !usb-redir.

If we want to extend this concept of fe open, let's do it right.  Let's
make all char frontends explicitly open, close on reset, etc.

Let's do something that makes sense across the board.

>
> Problem is the chardev forgets this state over migration.
>
> So we need a way to restore the state.  virtio-serial knows it of course
> as this is guest state it has to keep track of it.  We just need a way
> the chardev can figure what the current state is after migration.
>
> The options I see:
>
>   (1) Have chardev store the state itself in a new migration section.
>       This is what I've NAK'ed.  First, because live-migration host
>       state is a can of worms I don't want open.  Second because it
>       actually isn't host state.
>
>   (2) virtio-serial could just call qemu_chr_fe_open in
>       post_migration hook.  Could have unwanted side effects as
>       the chardev can't figure whenever this is a post-load call
>       or a guest-open call.

The only implementation of qemu_chr_fe_open() is:

static void spice_chr_guest_open(struct CharDriverState *chr)
{
    SpiceCharDriver *s = chr->opaque;
    vmc_register_interface(s);
}

Is this what you would do anyway?

In a world where qemu_chr_fe_open() was actually used throughout the
device model, all devices would need to have a post_load()
implementation where they checked their state to determine whether to
generate an open() call.

Regards,

Anthony Liguori

>
>   (3) Add a new qemu_chr_fe_* call for virtio-serial to notify the
>       chardev.  This was the next proposal from Alon
>
>   (4) Do it the other way around:  Allow the chardev query what the
>       current state is.  This patch series.
>
>   (5) Cut off the chardev layer altogether and implement spicevmc as
>       virtio-serial port.  Your proposal if I understand it correctly.
>       Makes sense, but breaks backward compatibility, so even when
>       doing this we must fix the chardev spicevmc bug somehow.
>
> Hope this clarifies,
>   Gerd
Anthony Liguori - March 22, 2013, 1:50 p.m.
Hans de Goede <hdegoede@redhat.com> writes:

> Hi,
>
> On 03/21/2013 07:18 PM, Anthony Liguori wrote:
>> Alon Levy <alevy@redhat.com> writes:
>>
>>> Note that the handler is called chr_is_guest_connected and not
>>> chr_is_fe_connected, consistent with other members of CharDriverState.
>>
>> Sorry, I don't get it.
>>
>> There isn't a notion of "connected" for the front-ends in the char
>> layer.
>
> And that is simply completely and utterly wrong. We've tried to explain
> this to you a number of times already. Yet you claim we've not explained
> it. Or we've not explained it properly. I must say however that I'm
> getting the feeling the problem is not us not explaining, but that you
> are not listening.

As usual, you make way too many assumptions without stopping to actually
think about what I'm saying.

> Still let me try to explain it 1 more time, in 2 different ways:
>
> 1) At an abstract level a chardev fe + be is a pipe between somewhere
> and where-ever. Both ends of the pipe can be opened or closed, not just
> one.

No, this is not the case in reality.  The notion of the front-end being
open or closed only exists for virtio-serial, usb-redir, and spice-*.

For every other aspect of the character device layer, the concept
doesn't exist.  We should have never allowed that in the first place and
I object strongly to extending the concept without making it make sense
for everything else.

> Frontends end inside the guest usually in the form of some piece of
> virtual hardware inside the guest. Just because the virtual hardware
> is there does not mean the guest has a driver,

Okay, let's use your example here with a standard UART.  In the
following sequence, I should receive:

1) Starts guest
2) When guest initializes the UART, qemu_chr_fe_open()
3) Reboot guest
4) Receive qemu_chr_fe_close()
5) Boot new guest without a UART driver
6) Nothing is received

But this doesn't happen today because the only backend that cares
(spice-*) assumes qemu_chr_fe_open() on the first qemu_chr_fe_write().
So it will certainly approximate steps 1-3 but will not behave correct
with steps 4-6.

Likewise, if you are dealing with something like a PCI hotpluggable UART
and the card is ejected (but the CDS isn't deleted), you won't receive a
close() event either.

Even just in the world of spice-* which is all ya'll seem to care about,
the behavior is broken today.

So before we go adding more hacks just for virtio-serial/spice-*, let's
consider how this applies to all character devices and do what makes
sense for all of them.

And for me, the most logical thing is to call qemu_chr_fe_open() in
post_load for the device.

Regards,

Anthony Liguori

> if the guest has
> a driver that creates a device-node for it, that does not mean there
> is a userspace process inside the guest which actually has the
> device-node open. So you see the front-end device-node inside the guest
> can be opened and closed. Most frontends cannot signal this to the
> backend but virtio-console can, and we want to know about it since
> we want to know if the user-space agent is there.



>
> 2) At the code level it clearly is there too, backend open-close
> is signalled to the frontend by means of the backend calling
> qemu_chr_be_event with an event of CHR_EVENT_OPENED and
> CHR_EVENT_CLOSED. Frontend open-close is signalled by the
> frontend calling qemu_chr_fe_open and qemu_chr_fe_close.
>
> Now the problem we have is that after migration the CHR_EVENT_OPENED
> gets replayed on the destination side but the qemu_chr_fe_open call
> does not.
>
> The logical place to replay the qemu_chr_fe_open would be in the
> frontend's migration handling code, as suggested here:
> http://lists.gnu.org/archive/html/qemu-devel/2012-11/msg02721.html
>
> But Amit did not like this approach, suggesting that instead we
> took care of this inside spice-qemu-char.c. Which is what we're
> trying to do now, but this requires spice-qemu-char.c being
> able to query the open state of the frontend, which is already
> being migrate by the virtio-console code in the form of the
> guest_connected variable, we just need to be able to get to that
> variable from the backend.
>
> Regards,
>
> Hans
Gerd Hoffmann - March 22, 2013, 3:53 p.m.
Hi,

> Okay, let's use your example here with a standard UART.  In the
> following sequence, I should receive:
> 
> 1) Starts guest
> 2) When guest initializes the UART, qemu_chr_fe_open()
> 3) Reboot guest
> 4) Receive qemu_chr_fe_close()
> 5) Boot new guest without a UART driver
> 6) Nothing is received

Well, with virtio-serial the logic is slightly different.
qemu_chr_fe_open() is called when a process opens
/dev/virtio-ports/$name, not when the virtio-serial driver loads.

I'm not sure whenever the qemu uart emulation can reliable do the same.
 Guest loading the uart driver can probably detected without too much
trouble.  But detecting a process opening /dev/ttySx might not be
possible.  Depends on the guest driver implementation.  For
virtio-serial it is trivial, there is a control message for that ;)

> And for me, the most logical thing is to call qemu_chr_fe_open() in
> post_load for the device.

Ok, just lets do that then.

cheers,
  Gerd
Hans de Goede - March 22, 2013, 4:50 p.m.
Hi,

On 03/22/2013 02:50 PM, Anthony Liguori wrote:
> Hans de Goede <hdegoede@redhat.com> writes:
>
>> Hi,
>>
>> On 03/21/2013 07:18 PM, Anthony Liguori wrote:
>>> Alon Levy <alevy@redhat.com> writes:
>>>
>>>> Note that the handler is called chr_is_guest_connected and not
>>>> chr_is_fe_connected, consistent with other members of CharDriverState.
>>>
>>> Sorry, I don't get it.
>>>
>>> There isn't a notion of "connected" for the front-ends in the char
>>> layer.
>>
>> And that is simply completely and utterly wrong. We've tried to explain
>> this to you a number of times already. Yet you claim we've not explained
>> it. Or we've not explained it properly. I must say however that I'm
>> getting the feeling the problem is not us not explaining, but that you
>> are not listening.
>
> As usual, you make way too many assumptions without stopping to actually
> think about what I'm saying.
>
>> Still let me try to explain it 1 more time, in 2 different ways:
>>
>> 1) At an abstract level a chardev fe + be is a pipe between somewhere
>> and where-ever. Both ends of the pipe can be opened or closed, not just
>> one.
>
> No, this is not the case in reality.

A pipe does not have 2 ends in reality ?

> The notion of the front-end being
> open or closed only exists for virtio-serial, usb-redir, and spice-*.
>
> For every other aspect of the character device layer, the concept
> doesn't exist.

The notion / concept of both ends of the pipe being opened and closed
certainly does exist. See your own example of a plain 16x50 uart and
the guest using it or not using it. Just because we cannot reliable /
practically detect the open/close does not mean the concept of it is not
there.

>  We should have never allowed that in the first place and
> I object strongly to extending the concept without making it make sense
> for everything else.
>
>> Frontends end inside the guest usually in the form of some piece of
>> virtual hardware inside the guest. Just because the virtual hardware
>> is there does not mean the guest has a driver,
>
> Okay, let's use your example here with a standard UART.  In the
> following sequence, I should receive:
>
> 1) Starts guest
> 2) When guest initializes the UART, qemu_chr_fe_open()
> 3) Reboot guest
> 4) Receive qemu_chr_fe_close()
> 5) Boot new guest without a UART driver
> 6) Nothing is received
>
> But this doesn't happen today because the only backend that cares
> (spice-*) assumes qemu_chr_fe_open() on the first qemu_chr_fe_write().

1) If the guest does not have an uart driver, nothing will be written
to the uart, so it wont call qemu_chr_fe_write and we won't assume
a qemu_chr_fe_open

2) But I agree the assuming of qemu_chr_fe_open on the first write is
a hack, it stems from before we had qemu_chr_fe_open/_close and now that
we do have we should remove it.

Note btw that many backends also don't handle sending CHR_EVENT_OPENED /
_CLOSED themselves, they simply use qemu_chr_generic_open and that generated
the opened event once on creation of the device. So the concept is just
as broken / not broken on the backend side.

We could do the same and have a qemu_fe_generic_open for frontends which
does the qemu_chr_fe_open call.

<snip>

> And for me, the most logical thing is to call qemu_chr_fe_open() in
> post_load for the device.

With the device I assume you mean the frontend? That is what we originally
suggested and submitted a patch for (for virtio-console) but Amit didn't
like it.

Regards,

Hans
Anthony Liguori - March 22, 2013, 5:11 p.m.
Hans de Goede <hdegoede@redhat.com> writes:

> Hi,
>
> On 03/22/2013 02:50 PM, Anthony Liguori wrote:
>> Hans de Goede <hdegoede@redhat.com> writes:
>>
>>  We should have never allowed that in the first place and
>> I object strongly to extending the concept without making it make sense
>> for everything else.
>>
>>> Frontends end inside the guest usually in the form of some piece of
>>> virtual hardware inside the guest. Just because the virtual hardware
>>> is there does not mean the guest has a driver,
>>
>> Okay, let's use your example here with a standard UART.  In the
>> following sequence, I should receive:
>>
>> 1) Starts guest
>> 2) When guest initializes the UART, qemu_chr_fe_open()
>> 3) Reboot guest
>> 4) Receive qemu_chr_fe_close()
>> 5) Boot new guest without a UART driver
>> 6) Nothing is received
>>
>> But this doesn't happen today because the only backend that cares
>> (spice-*) assumes qemu_chr_fe_open() on the first qemu_chr_fe_write().
>
> 1) If the guest does not have an uart driver, nothing will be written
> to the uart, so it wont call qemu_chr_fe_write and we won't assume
> a qemu_chr_fe_open
>
> 2) But I agree the assuming of qemu_chr_fe_open on the first write is
> a hack, it stems from before we had qemu_chr_fe_open/_close and now that
> we do have we should remove it.

If the qemu-char.c code did:

int qemu_chr_fe_write(...) {
    if (!s->fe_open) {
        qemu_chr_fe_open(s);
    }
    ...
}

That would be one thing.  It's a hack, but a more reasonable hack than
doing this in the backend like we do today.

And in fact, herein lies exactly what the problem is.  There is no
"s->fe_open".  And if such a thing did exist, we would note that this is
in fact guest state and that it needs to be taken care of during
migration.

> Note btw that many backends also don't handle sending CHR_EVENT_OPENED /
> _CLOSED themselves, they simply use qemu_chr_generic_open and that generated
> the opened event once on creation of the device. So the concept is just
> as broken / not broken on the backend side.

I know :-/

> We could do the same and have a qemu_fe_generic_open for frontends which
> does the qemu_chr_fe_open call.

You mean, in qemu_chr_fe_write()?  Yes, I think not doing this bit in
the backend and making it part of qemu-char would be a significant
improvement and would also guide in solving this problem correctly.

Also, you can get reset handling for free by unconditionally clearly
s->fe_open with a reset handler.  That would also simplify the
virtio-serial code since it would no longer need the reset logic.

>> And for me, the most logical thing is to call qemu_chr_fe_open() in
>> post_load for the device.
>
> With the device I assume you mean the frontend? That is what we originally
> suggested and submitted a patch for (for virtio-console) but Amit didn't
> like it.

As Avi would say, this is "derived guest state" and derived guest state
has to be recomputed in post_load handlers.

Regards,

Anthony Liguori

>
> Regards,
>
> Hans
Hans de Goede - March 24, 2013, 12:37 p.m.
Hi,

On 03/22/2013 06:11 PM, Anthony Liguori wrote:
> Hans de Goede <hdegoede@redhat.com> writes:

<snip>

> If the qemu-char.c code did:
>
> int qemu_chr_fe_write(...) {
>      if (!s->fe_open) {
>          qemu_chr_fe_open(s);
>      }
>      ...
> }
>
> That would be one thing.  It's a hack, but a more reasonable hack than
> doing this in the backend like we do today.

Agreed.

> And in fact, herein lies exactly what the problem is.  There is no
> "s->fe_open".  And if such a thing did exist, we would note that this is
> in fact guest state and that it needs to be taken care of during
> migration.

Agreed too, I've prepared a patch set adding fe_open and cleaning up the
whole existing code around the fe_open stuff. I'll send it directly after
this mail.

<snip>

>> We could do the same and have a qemu_fe_generic_open for frontends which
>> does the qemu_chr_fe_open call.
>
> You mean, in qemu_chr_fe_write()?  Yes, I think not doing this bit in
> the backend and making it part of qemu-char would be a significant
> improvement and would also guide in solving this problem correctly.

I believe that qemu_chr_fe_write is too late, this assumes that the
frontend is always the first one to do a write (assuming fe_open aware
backends won't do anything until the fe_open happens). But what if the
protocol going over the chardev expects the backend to do the first
write? Then the backend will just sit there waiting for the fe_open,
and the frontend will just sit there waiting for the first write before
sending this fe_open.

I believe that your previous comments on qemu_chr_add_handlers being
the closest thing to a fe_open for many frontends is correct, esp. since
some frontends and hw/qdev-properties-system.c do a NULL
qemu_chr_add_handlers when a frontend is being unregistered / removed /
closed. Which gives us a central place to detect frontend closes too.

So this is what I've used in my patch-set.

> Also, you can get reset handling for free by unconditionally clearly
> s->fe_open with a reset handler.  That would also simplify the
> virtio-serial code since it would no longer need the reset logic.
>
>>> And for me, the most logical thing is to call qemu_chr_fe_open() in
>>> post_load for the device.
>>
>> With the device I assume you mean the frontend? That is what we originally
>> suggested and submitted a patch for (for virtio-console) but Amit didn't
>> like it.
>
> As Avi would say, this is "derived guest state" and derived guest state
> has to be recomputed in post_load handlers.

Agreed, which brings us back to the original patch posted a long time
ago which Amit did not like:
http://lists.gnu.org/archive/html/qemu-devel/2012-11/msg02721.html

Amit can you take a second look at this? Note that after the cleanup patchset
which I'll send right after this mail, it will look slightly different, something
like:

@@ -653,6 +654,10 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
              send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
                                 port->host_connected);
          }
+        vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+        if (vsc->set_guest_connected) {
+            vsc->set_guest_connected(port, port->guest_connected);
+        }
      }
      g_free(s->post_load.connected);
      s->post_load.connected = NULL;

Which to me looks like a reasonable thing to do on post-load.

Regards,

Hans

Patch

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index e2d1c58..643e24e 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -120,6 +120,13 @@  static void chr_event(void *opaque, int event)
     }
 }
 
+static bool chr_is_guest_connected(void *opaque)
+{
+    VirtConsole *vcon = opaque;
+
+    return vcon->port.guest_connected;
+}
+
 static int virtconsole_initfn(VirtIOSerialPort *port)
 {
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
@@ -133,6 +140,8 @@  static int virtconsole_initfn(VirtIOSerialPort *port)
     if (vcon->chr) {
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
                               vcon);
+        /* only user of chr_is_guest_connected so leave it as special cased*/
+        vcon->chr->chr_is_guest_connected = chr_is_guest_connected;
     }
 
     return 0;
diff --git a/include/char/char.h b/include/char/char.h
index 0326b2a..b41ddc0 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -52,6 +52,7 @@  typedef struct {
 #define CHR_TIOCM_RTS	0x004
 
 typedef void IOEventHandler(void *opaque, int event);
+typedef bool IOIsGuestConnectedHandler(void *opaque);
 
 struct CharDriverState {
     void (*init)(struct CharDriverState *s);
@@ -64,6 +65,7 @@  struct CharDriverState {
     IOEventHandler *chr_event;
     IOCanReadHandler *chr_can_read;
     IOReadHandler *chr_read;
+    IOIsGuestConnectedHandler *chr_is_guest_connected;
     void *handler_opaque;
     void (*chr_close)(struct CharDriverState *chr);
     void (*chr_accept_input)(struct CharDriverState *chr);
@@ -229,6 +231,15 @@  void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len);
  */
 void qemu_chr_be_event(CharDriverState *s, int event);
 
+/**
+ * @qemu_chr_be_is_fe_connected:
+ *
+ * Back end calls this to check if the front end is connected.
+ *
+ * Returns: true if the guest (front end) is connected, false otherwise.
+ */
+bool qemu_chr_be_is_fe_connected(CharDriverState *s);
+
 void qemu_chr_add_handlers(CharDriverState *s,
                            IOCanReadHandler *fd_can_read,
                            IOReadHandler *fd_read,
diff --git a/qemu-char.c b/qemu-char.c
index 4e011df..77a501a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -120,6 +120,15 @@  void qemu_chr_be_event(CharDriverState *s, int event)
     s->chr_event(s->handler_opaque, event);
 }
 
+bool qemu_chr_be_is_fe_connected(CharDriverState *s)
+{
+    if (s->chr_is_guest_connected) {
+        return s->chr_is_guest_connected(s->handler_opaque);
+    }
+    /* default to always connected */
+    return true;
+}
+
 static gboolean qemu_chr_generic_open_bh(gpointer opaque)
 {
     CharDriverState *s = opaque;