Patchwork [1/4] char: add a post_load callback

login
register
mail settings
Submitter Alon Levy
Date March 20, 2013, 9:55 a.m.
Message ID <1363773314-32332-2-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/229301/
State New
Headers show

Comments

Alon Levy - March 20, 2013, 9:55 a.m.
Signed-off-by: Alon Levy <alevy@redhat.com>
---
 include/char/char.h | 12 ++++++++++++
 qemu-char.c         |  7 +++++++
 2 files changed, 19 insertions(+)
Anthony Liguori - March 20, 2013, 1:08 p.m.
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 - March 20, 2013, 4:59 p.m.
> 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 - March 20, 2013, 5:05 p.m.
> 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
> 
> 
>
Anthony Liguori - March 20, 2013, 6:59 p.m.
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
>> 
>> 
>>
Gerd Hoffmann - March 21, 2013, 6:53 a.m.
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
Hans de Goede - March 21, 2013, 8:27 a.m.
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
Hans de Goede - March 21, 2013, 8:36 a.m.
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
Alon Levy - March 21, 2013, 8:54 a.m.
> 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
> 
> 
> 
>
Alon Levy - March 21, 2013, 4:35 p.m.
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(+)

Patch

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