[1/2] char: add qemu_chr_be_is_fe_connected

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

Hint will be taken into future commit messages.

Actually it already exists in the last commit message: currently, when the migration target finishing migration and enters the running state, the spice server has never received any indication that the guest agent is alive, and so it assumes it isn't. In the source machine, this is accomplished by the chr_guest_open callback implemented by spice_chr_guest_open. To make sure the target does see this event, the second patch adds a post_load for spicevmc/spiceport that will check the front end connected state and call spice_chr_guest_open if the front end is connected. spicevmc is the back end in this case, virtio-console is the front end.

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

There are guest open and guest closed callbacks in the generic character device interface. This is merely adding a convenient state that could be inferred by reading the history of those calls. In what way is it new or pretend?

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

spicevmc is used by usbredir and ccid-card-passthru in addition to virtio-console. The bug/problem I am trying to solve is however only happening with the vdagent interface that uses virtio-console at the moment. It is not strange to assume it will use something else at some point, since it only requires a transport. It matches with the char device interface very well.

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

It does make sense, since it is just tracking chr_guest_open & chr_guest_close history. But in order to work across migration it needs to have vmstate. Is vmstate in the chardev layer acceptable, something like the patch at the end of this mail?

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

Perhaps I'm missing something, or it's a non problem. I'm waiting for Gerd to explain it better then me since he raised it.

I didn't mean identical, that was a mistake - I meant they would have identifiers that are only distinguished by the order the corresponding "-chardev" appeared on the command line. So if the target vm had the two chardevs (that are connected to different devices) reversed, it could load the wrong state to both.

> 
> Regards,
> 
> Anthony Liguori

Introducing fe_opened vmstate for replay of chr_guest_open for spice-qemu-char chardev (the second patch remains the same other then the renamed api to qemu_chr_be_is_fe_open): (this comes on top of a patch I omitted that renames CharDeviceState->opened to be_opened).

commit 35f3ce9dbaaa5059e8100c779eedbc0bf5bb98d2
Author: Alon Levy <alevy@redhat.com>
Date:   Thu Mar 21 17:24:00 2013 +0200

    char: add qemu_chr_be_is_fe_open
    
    This function returns the current open state of the guest, it tracks the
    existing fe called functions qemu_chr_fe_open and qemu_chr_fe_close,
    including adding vmstate to track this across migration.
    
    Signed-off-by: Alon Levy <alevy@redhat.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.
> 
> Hint will be taken into future commit messages.
> 
> Actually it already exists in the last commit message: currently,
> when the migration target finishing migration and enters the running
> state, the spice server has never received any indication that the
> guest agent is alive, and so it assumes it isn't. In the source
> machine, this is accomplished by the chr_guest_open callback
> implemented by spice_chr_guest_open. To make sure the target does
> see this event, the second patch adds a post_load for
> spicevmc/spiceport that will check the front end connected state and
> call spice_chr_guest_open if the front end is connected. spicevmc is
> the back end in this case, virtio-console is the front end.
> 
> > 
> > >> 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.
> 
> There are guest open and guest closed callbacks in the generic
> character device interface. This is merely adding a convenient state
> that could be inferred by reading the history of those calls. In
> what way is it new or pretend?
> 
> > 
> > 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.
> 
> spicevmc is used by usbredir and ccid-card-passthru in addition to
> virtio-console. The bug/problem I am trying to solve is however only
> happening with the vdagent interface that uses virtio-console at the
> moment. It is not strange to assume it will use something else at
> some point, since it only requires a transport. It matches with the
> char device interface very well.
> 
> > 
> > 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.
> 
> It does make sense, since it is just tracking chr_guest_open &
> chr_guest_close history. But in order to work across migration it
> needs to have vmstate. Is vmstate in the chardev layer acceptable,
> something like the patch at the end of this mail?
> 
> >
> > >> 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.
> 
> Perhaps I'm missing something, or it's a non problem. I'm waiting for
> Gerd to explain it better then me since he raised it.
> 
> I didn't mean identical, that was a mistake - I meant they would have
> identifiers that are only distinguished by the order the
> corresponding "-chardev" appeared on the command line. So if the
> target vm had the two chardevs (that are connected to different
> devices) reversed, it could load the wrong state to both.
> 
> > 
> > Regards,
> > 
> > Anthony Liguori
> 
> Introducing fe_opened vmstate for replay of chr_guest_open for
> spice-qemu-char chardev (the second patch remains the same other
> then the renamed api to qemu_chr_be_is_fe_open): (this comes on top
> of a patch I omitted that renames CharDeviceState->opened to
> be_opened).
> 
> commit 35f3ce9dbaaa5059e8100c779eedbc0bf5bb98d2
> Author: Alon Levy <alevy@redhat.com>
> Date:   Thu Mar 21 17:24:00 2013 +0200
> 
>     char: add qemu_chr_be_is_fe_open
>     
>     This function returns the current open state of the guest, it
>     tracks the
>     existing fe called functions qemu_chr_fe_open and
>     qemu_chr_fe_close,
>     including adding vmstate to track this across migration.
>     
>     Signed-off-by: Alon Levy <alevy@redhat.com>
> 
> diff --git a/include/char/char.h b/include/char/char.h
> index 26bc174..fb893a8 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);

Oops, the above line should have been dropped.

>  
>  struct CharDriverState {
>      void (*init)(struct CharDriverState *s);
> @@ -75,6 +76,7 @@ struct CharDriverState {
>      char *label;
>      char *filename;
>      int be_opened;
> +    int fe_opened;
>      int avail_connections;
>      QemuOpts *opts;
>      QTAILQ_ENTRY(CharDriverState) next;
> @@ -229,6 +231,16 @@ 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_open:
> + *
> + * Back end calls this to check if the front end is connected.
> + *
> + * Returns: true if the guest (front end) is open, that
> chr_guest_open has
> + * been the last call, and not chr_guest_close.
> + */
> +int qemu_chr_be_is_fe_open(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 2a1a084..8379f9c 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -120,6 +120,11 @@ void qemu_chr_be_event(CharDriverState *s, int
> event)
>      s->chr_event(s->handler_opaque, event);
>  }
>  
> +int qemu_chr_be_is_fe_open(CharDriverState *s)
> +{
> +    return s->fe_opened;
> +}
> +
>  static gboolean qemu_chr_generic_open_bh(gpointer opaque)
>  {
>      CharDriverState *s = opaque;
> @@ -3385,6 +3390,7 @@ void qemu_chr_fe_set_echo(struct
> CharDriverState *chr, bool echo)
>  
>  void qemu_chr_fe_open(struct CharDriverState *chr)
>  {
> +    chr->fe_opened = 1;
>      if (chr->chr_guest_open) {
>          chr->chr_guest_open(chr);
>      }
> @@ -3392,6 +3398,7 @@ void qemu_chr_fe_open(struct CharDriverState
> *chr)
>  
>  void qemu_chr_fe_close(struct CharDriverState *chr)
>  {
> +    chr->fe_opened = 0;
>      if (chr->chr_guest_close) {
>          chr->chr_guest_close(chr);
>      }
> @@ -3684,6 +3691,17 @@ static CharDriverState
> *qmp_chardev_open_dgram(ChardevDgram *dgram,
>      return qemu_chr_open_udp_fd(fd);
>  }
>  
> +static VMStateDescription qemu_chardev_vmstate = {
> +    .name               = "qemu-chardev",
> +    .version_id         = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT32(fe_opened, CharDriverState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +
>  ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend
>  *backend,
>                                 Error **errp)
>  {
> @@ -3775,6 +3793,7 @@ ChardevReturn *qmp_chardev_add(const char *id,
> ChardevBackend *backend,
>          chr->label = g_strdup(id);
>          chr->avail_connections =
>              (backend->kind == CHARDEV_BACKEND_KIND_MUX) ? MAX_MUX :
>              1;
> +        vmstate_register(NULL, -1, &qemu_chardev_vmstate, chr);
>          QTAILQ_INSERT_TAIL(&chardevs, chr, next);
>          return ret;
>      } else {
> 
>