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

[Alon Levy]

> > Alon Levy <address@hidden> writes:
> > 
> > >> Alon Levy <address@hidden> 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 <address@hidden>
> 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 <address@hidden>
> 
> 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 {
> 
>