| Message ID | 1363773314-32332-2-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
Alon Levy <alevy@redhat.com> writes: > Signed-off-by: Alon Levy <alevy@redhat.com> > --- > include/char/char.h | 12 ++++++++++++ > qemu-char.c | 7 +++++++ > 2 files changed, 19 insertions(+) > > diff --git a/include/char/char.h b/include/char/char.h > index 0326b2a..0fdcaf9 100644 > --- a/include/char/char.h > +++ b/include/char/char.h > @@ -70,6 +70,7 @@ struct CharDriverState { > void (*chr_set_echo)(struct CharDriverState *chr, bool echo); > void (*chr_guest_open)(struct CharDriverState *chr); > void (*chr_guest_close)(struct CharDriverState *chr); > + void (*chr_post_load)(struct CharDriverState *chr, int > connected); The character device layer should *not* be messing around with notifying migration state. I thought we previously discussed this? Just implement a migration hook in the spice code. Regards, Anthony Liguori > void *opaque; > int idle_tag; > char *label; > @@ -144,6 +145,17 @@ void qemu_chr_fe_open(struct CharDriverState *chr); > void qemu_chr_fe_close(struct CharDriverState *chr); > > /** > + * @qemu_chr_fe_post_load: > + * > + * Indicate to backend that a migration has just completed. Must be called when > + * the vm is in the running state. > + * > + * @connected true if frontend is still connected after migration, false > + * otherwise. > + */ > +void qemu_chr_fe_post_load(struct CharDriverState *chr, int connected); > + > +/** > * @qemu_chr_fe_printf: > * > * Write to a character backend using a printf style interface. > diff --git a/qemu-char.c b/qemu-char.c > index 4e011df..42c911f 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -3390,6 +3390,13 @@ void qemu_chr_fe_open(struct CharDriverState *chr) > } > } > > +void qemu_chr_fe_post_load(struct CharDriverState *chr, int connected) > +{ > + if (chr->chr_post_load) { > + chr->chr_post_load(chr, connected); > + } > +} > + > void qemu_chr_fe_close(struct CharDriverState *chr) > { > if (chr->chr_guest_close) { > -- > 1.8.1.4
> Alon Levy <alevy@redhat.com> writes: > > > Signed-off-by: Alon Levy <alevy@redhat.com> > > --- > > include/char/char.h | 12 ++++++++++++ > > qemu-char.c | 7 +++++++ > > 2 files changed, 19 insertions(+) > > > > diff --git a/include/char/char.h b/include/char/char.h > > index 0326b2a..0fdcaf9 100644 > > --- a/include/char/char.h > > +++ b/include/char/char.h > > @@ -70,6 +70,7 @@ struct CharDriverState { > > void (*chr_set_echo)(struct CharDriverState *chr, bool echo); > > void (*chr_guest_open)(struct CharDriverState *chr); > > void (*chr_guest_close)(struct CharDriverState *chr); > > + void (*chr_post_load)(struct CharDriverState *chr, int > > connected); > > The character device layer should *not* be messing around with > notifying > migration state. > > I thought we previously discussed this? Just implement a migration > hook > in the spice code. We did and Gerd objected so I sent this to have this discussion again, with him. > > Regards, > > Anthony Liguori > > > void *opaque; > > int idle_tag; > > char *label; > > @@ -144,6 +145,17 @@ void qemu_chr_fe_open(struct CharDriverState > > *chr); > > void qemu_chr_fe_close(struct CharDriverState *chr); > > > > /** > > + * @qemu_chr_fe_post_load: > > + * > > + * Indicate to backend that a migration has just completed. Must > > be called when > > + * the vm is in the running state. > > + * > > + * @connected true if frontend is still connected after migration, > > false > > + * otherwise. > > + */ > > +void qemu_chr_fe_post_load(struct CharDriverState *chr, int > > connected); > > + > > +/** > > * @qemu_chr_fe_printf: > > * > > * Write to a character backend using a printf style interface. > > diff --git a/qemu-char.c b/qemu-char.c > > index 4e011df..42c911f 100644 > > --- a/qemu-char.c > > +++ b/qemu-char.c > > @@ -3390,6 +3390,13 @@ void qemu_chr_fe_open(struct CharDriverState > > *chr) > > } > > } > > > > +void qemu_chr_fe_post_load(struct CharDriverState *chr, int > > connected) > > +{ > > + if (chr->chr_post_load) { > > + chr->chr_post_load(chr, connected); > > + } > > +} > > + > > void qemu_chr_fe_close(struct CharDriverState *chr) > > { > > if (chr->chr_guest_close) { > > -- > > 1.8.1.4 > >
> Alon Levy <alevy@redhat.com> writes: > > > Signed-off-by: Alon Levy <alevy@redhat.com> > > --- > > include/char/char.h | 12 ++++++++++++ > > qemu-char.c | 7 +++++++ > > 2 files changed, 19 insertions(+) > > > > diff --git a/include/char/char.h b/include/char/char.h > > index 0326b2a..0fdcaf9 100644 > > --- a/include/char/char.h > > +++ b/include/char/char.h > > @@ -70,6 +70,7 @@ struct CharDriverState { > > void (*chr_set_echo)(struct CharDriverState *chr, bool echo); > > void (*chr_guest_open)(struct CharDriverState *chr); > > void (*chr_guest_close)(struct CharDriverState *chr); > > + void (*chr_post_load)(struct CharDriverState *chr, int > > connected); > > The character device layer should *not* be messing around with > notifying > migration state. > > I thought we previously discussed this? Just implement a migration > hook > in the spice code. The thing Gerd objected to when I sent a patch doing just that was the way I used the vmstate, one possible way to not have to use vmstate at all is adding api for querying the current front end connected status, like qemu_fe_is_connected. Is that acceptable? > > Regards, > > Anthony Liguori > > > void *opaque; > > int idle_tag; > > char *label; > > @@ -144,6 +145,17 @@ void qemu_chr_fe_open(struct CharDriverState > > *chr); > > void qemu_chr_fe_close(struct CharDriverState *chr); > > > > /** > > + * @qemu_chr_fe_post_load: > > + * > > + * Indicate to backend that a migration has just completed. Must > > be called when > > + * the vm is in the running state. > > + * > > + * @connected true if frontend is still connected after migration, > > false > > + * otherwise. > > + */ > > +void qemu_chr_fe_post_load(struct CharDriverState *chr, int > > connected); > > + > > +/** > > * @qemu_chr_fe_printf: > > * > > * Write to a character backend using a printf style interface. > > diff --git a/qemu-char.c b/qemu-char.c > > index 4e011df..42c911f 100644 > > --- a/qemu-char.c > > +++ b/qemu-char.c > > @@ -3390,6 +3390,13 @@ void qemu_chr_fe_open(struct CharDriverState > > *chr) > > } > > } > > > > +void qemu_chr_fe_post_load(struct CharDriverState *chr, int > > connected) > > +{ > > + if (chr->chr_post_load) { > > + chr->chr_post_load(chr, connected); > > + } > > +} > > + > > void qemu_chr_fe_close(struct CharDriverState *chr) > > { > > if (chr->chr_guest_close) { > > -- > > 1.8.1.4 > > >
Alon Levy <alevy@redhat.com> writes: >> Alon Levy <alevy@redhat.com> writes: >> >> > Signed-off-by: Alon Levy <alevy@redhat.com> >> > --- >> > include/char/char.h | 12 ++++++++++++ >> > qemu-char.c | 7 +++++++ >> > 2 files changed, 19 insertions(+) >> > >> > diff --git a/include/char/char.h b/include/char/char.h >> > index 0326b2a..0fdcaf9 100644 >> > --- a/include/char/char.h >> > +++ b/include/char/char.h >> > @@ -70,6 +70,7 @@ struct CharDriverState { >> > void (*chr_set_echo)(struct CharDriverState *chr, bool echo); >> > void (*chr_guest_open)(struct CharDriverState *chr); >> > void (*chr_guest_close)(struct CharDriverState *chr); >> > + void (*chr_post_load)(struct CharDriverState *chr, int >> > connected); >> >> The character device layer should *not* be messing around with >> notifying >> migration state. >> >> I thought we previously discussed this? Just implement a migration >> hook >> in the spice code. > > The thing Gerd objected to when I sent a patch doing just that was the > way I used the vmstate, one possible way to not have to use vmstate at > all is adding api for querying the current front end connected status, > like qemu_fe_is_connected. Is that acceptable? To determine if the backend is connected? If so, it's fine, but I'd suggest being more explicit and calling it qemu_fe_is_be_connected(). Regards, Anthony Liguori > >> >> Regards, >> >> Anthony Liguori >> >> > void *opaque; >> > int idle_tag; >> > char *label; >> > @@ -144,6 +145,17 @@ void qemu_chr_fe_open(struct CharDriverState >> > *chr); >> > void qemu_chr_fe_close(struct CharDriverState *chr); >> > >> > /** >> > + * @qemu_chr_fe_post_load: >> > + * >> > + * Indicate to backend that a migration has just completed. Must >> > be called when >> > + * the vm is in the running state. >> > + * >> > + * @connected true if frontend is still connected after migration, >> > false >> > + * otherwise. >> > + */ >> > +void qemu_chr_fe_post_load(struct CharDriverState *chr, int >> > connected); >> > + >> > +/** >> > * @qemu_chr_fe_printf: >> > * >> > * Write to a character backend using a printf style interface. >> > diff --git a/qemu-char.c b/qemu-char.c >> > index 4e011df..42c911f 100644 >> > --- a/qemu-char.c >> > +++ b/qemu-char.c >> > @@ -3390,6 +3390,13 @@ void qemu_chr_fe_open(struct CharDriverState >> > *chr) >> > } >> > } >> > >> > +void qemu_chr_fe_post_load(struct CharDriverState *chr, int >> > connected) >> > +{ >> > + if (chr->chr_post_load) { >> > + chr->chr_post_load(chr, connected); >> > + } >> > +} >> > + >> > void qemu_chr_fe_close(struct CharDriverState *chr) >> > { >> > if (chr->chr_guest_close) { >> > -- >> > 1.8.1.4 >> >> >>
On 03/20/13 17:59, Alon Levy wrote: >> > I thought we previously discussed this? Just implement a migration >> > hook >> > in the spice code. > We did and Gerd objected so I sent this to have this discussion again, with him. migration section != migration hook. I think what Anthony asks for is to register a migration state notifier and handle the post-migration action there instead of adding a chardev callback. cheers, Gerd
Hi, On 03/20/2013 07:59 PM, Anthony Liguori wrote: > Alon Levy <alevy@redhat.com> writes: > >>> Alon Levy <alevy@redhat.com> writes: >>> >>>> Signed-off-by: Alon Levy <alevy@redhat.com> >>>> --- >>>> include/char/char.h | 12 ++++++++++++ >>>> qemu-char.c | 7 +++++++ >>>> 2 files changed, 19 insertions(+) >>>> >>>> diff --git a/include/char/char.h b/include/char/char.h >>>> index 0326b2a..0fdcaf9 100644 >>>> --- a/include/char/char.h >>>> +++ b/include/char/char.h >>>> @@ -70,6 +70,7 @@ struct CharDriverState { >>>> void (*chr_set_echo)(struct CharDriverState *chr, bool echo); >>>> void (*chr_guest_open)(struct CharDriverState *chr); >>>> void (*chr_guest_close)(struct CharDriverState *chr); >>>> + void (*chr_post_load)(struct CharDriverState *chr, int >>>> connected); >>> >>> The character device layer should *not* be messing around with >>> notifying >>> migration state. >>> >>> I thought we previously discussed this? Just implement a migration >>> hook >>> in the spice code. >> >> The thing Gerd objected to when I sent a patch doing just that was the >> way I used the vmstate, one possible way to not have to use vmstate at >> all is adding api for querying the current front end connected status, >> like qemu_fe_is_connected. Is that acceptable? > > To determine if the backend is connected? No to query if the front-end is connected to the guest, with virtio-ports just because they are there does not mean the guest is listening, so qemu_fe_is_connected is the right name, or maybe qemu_fe_is_guest_connected If so, it's fine, but I'd > suggest being more explicit and calling it qemu_fe_is_be_connected(). Definitely not qemu_fe_is_be_connected that would mean asking if a chardev backend is connected, which is not what we're interested in (we're calling this from a backend, so we know we're connected ourselves). Regards, Hans
Hi, On 03/21/2013 09:27 AM, Hans de Goede wrote: > Hi, > > On 03/20/2013 07:59 PM, Anthony Liguori wrote: >> Alon Levy <alevy@redhat.com> writes: >> >>>> Alon Levy <alevy@redhat.com> writes: >>>> >>>>> Signed-off-by: Alon Levy <alevy@redhat.com> >>>>> --- >>>>> include/char/char.h | 12 ++++++++++++ >>>>> qemu-char.c | 7 +++++++ >>>>> 2 files changed, 19 insertions(+) >>>>> >>>>> diff --git a/include/char/char.h b/include/char/char.h >>>>> index 0326b2a..0fdcaf9 100644 >>>>> --- a/include/char/char.h >>>>> +++ b/include/char/char.h >>>>> @@ -70,6 +70,7 @@ struct CharDriverState { >>>>> void (*chr_set_echo)(struct CharDriverState *chr, bool echo); >>>>> void (*chr_guest_open)(struct CharDriverState *chr); >>>>> void (*chr_guest_close)(struct CharDriverState *chr); >>>>> + void (*chr_post_load)(struct CharDriverState *chr, int >>>>> connected); >>>> >>>> The character device layer should *not* be messing around with >>>> notifying >>>> migration state. >>>> >>>> I thought we previously discussed this? Just implement a migration >>>> hook >>>> in the spice code. >>> >>> The thing Gerd objected to when I sent a patch doing just that was the >>> way I used the vmstate, one possible way to not have to use vmstate at >>> all is adding api for querying the current front end connected status, >>> like qemu_fe_is_connected. Is that acceptable? >> >> To determine if the backend is connected? > > No to query if the front-end is connected to the guest, with virtio-ports > just because they are there does not mean the guest is listening, > so qemu_fe_is_connected is the right name, or maybe > qemu_fe_is_guest_connected Hmm, wait Alon fell for the pitfall of the somewhat weird naming of the chardev functions, where functions which are prefixed with qemu_chr_fe are not calls *to* the frontend, but are functions intended to be called by the frontend (I'm used to functions being named after the subject, not after the caller). So what we would like to add is a new qemu_chr_be_is_fe_connected to be called by backends to find out if the frontend is connected (iow if the guest is actually listening to a virtio-port). This could then be called by the spicevmc be code from a vm_change_state_handler to find out if the guest is connected to the virtio-port post migration. Anthony, would that be an acceptable solution? Regards, Hans
> On 03/20/13 17:59, Alon Levy wrote: > >> > I thought we previously discussed this? Just implement a > >> > migration > >> > hook > >> > in the spice code. > > We did and Gerd objected so I sent this to have this discussion > > again, with him. > > migration section != migration hook. > > I think what Anthony asks for is to register a migration state > notifier > and handle the post-migration action there instead of adding a > chardev > callback. Yes, but then to know if the frontend (aka guest, aka virtio-console/virtio-serial-bus) is connected we need some api in the chardev layer, which is what I asked about. That way spice-qemu-char doesn't need to keep any vmstate. > > cheers, > Gerd > > > >
This series is back to not adding any generic post load callback to the char device layer, the only difference from the initial post is that no state is kept in spicevmc/spiceport and instead the guest connected status is queried from the device. v3: use qemu_chr_be_is_fe_connected instead of local state. Alon Levy (2): char: add qemu_chr_be_is_fe_connected spice-qemu-char: register interface on post load hw/virtio-console.c | 9 +++++++++ include/char/char.h | 11 +++++++++++ qemu-char.c | 9 +++++++++ spice-qemu-char.c | 39 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+)
diff --git a/include/char/char.h b/include/char/char.h index 0326b2a..0fdcaf9 100644 --- a/include/char/char.h +++ b/include/char/char.h @@ -70,6 +70,7 @@ struct CharDriverState { void (*chr_set_echo)(struct CharDriverState *chr, bool echo); void (*chr_guest_open)(struct CharDriverState *chr); void (*chr_guest_close)(struct CharDriverState *chr); + void (*chr_post_load)(struct CharDriverState *chr, int connected); void *opaque; int idle_tag; char *label; @@ -144,6 +145,17 @@ void qemu_chr_fe_open(struct CharDriverState *chr); void qemu_chr_fe_close(struct CharDriverState *chr); /** + * @qemu_chr_fe_post_load: + * + * Indicate to backend that a migration has just completed. Must be called when + * the vm is in the running state. + * + * @connected true if frontend is still connected after migration, false + * otherwise. + */ +void qemu_chr_fe_post_load(struct CharDriverState *chr, int connected); + +/** * @qemu_chr_fe_printf: * * Write to a character backend using a printf style interface. diff --git a/qemu-char.c b/qemu-char.c index 4e011df..42c911f 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -3390,6 +3390,13 @@ void qemu_chr_fe_open(struct CharDriverState *chr) } } +void qemu_chr_fe_post_load(struct CharDriverState *chr, int connected) +{ + if (chr->chr_post_load) { + chr->chr_post_load(chr, connected); + } +} + void qemu_chr_fe_close(struct CharDriverState *chr) { if (chr->chr_guest_close) {
Signed-off-by: Alon Levy <alevy@redhat.com> --- include/char/char.h | 12 ++++++++++++ qemu-char.c | 7 +++++++ 2 files changed, 19 insertions(+)