Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Anthony Liguori <aliguori@us.ibm.com>]

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
>> 
>> 
>>