Message ID | 57091189.12922836.1363902903515.JavaMail.root@redhat.com |
---|---|
State | New |
Headers | show |
> > 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 { > >
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); 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 {