Patchwork xen: introduce xen_change_state_handler

login
register
mail settings
Submitter Stefano Stabellini
Date June 29, 2011, 11:16 a.m.
Message ID <1309346169-14554-1-git-send-email-stefano.stabellini@eu.citrix.com>
Download mbox | patch
Permalink /patch/102570/
State New
Headers show

Comments

Stefano Stabellini - June 29, 2011, 11:16 a.m.
From: Anthony PERARD <anthony.perard@citrix.com>

Remove the call to xenstore_record_dm_state from xen_main_loop_prepare
that is HVM specific.
Add a new vm_change_state_handler shared between xen_pv and xen_hvm
machines to record the VM state to xenstore.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen-all.c |   25 ++++++++++++++++++-------
 1 files changed, 18 insertions(+), 7 deletions(-)
Alexander Graf - June 30, 2011, 12:11 p.m.
On 06/29/2011 01:16 PM, stefano.stabellini@eu.citrix.com wrote:
> From: Anthony PERARD<anthony.perard@citrix.com>
>
> Remove the call to xenstore_record_dm_state from xen_main_loop_prepare
> that is HVM specific.
> Add a new vm_change_state_handler shared between xen_pv and xen_hvm
> machines to record the VM state to xenstore.
>
> Signed-off-by: Anthony PERARD<anthony.perard@citrix.com>
> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> ---
>   xen-all.c |   25 ++++++++++++++++++-------
>   1 files changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/xen-all.c b/xen-all.c
> index 3fd04ef..e8da35f 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -797,12 +797,17 @@ void xenstore_store_pv_console_info(int i, CharDriverState *chr)
>       }
>   }
>
> -static void xenstore_record_dm_state(XenIOState *s, const char *state)
> +static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
>   {
>       char path[50];
>
> +    if (xs == NULL) {
> +        fprintf(stderr, "xenstore connection not initialized\n");
> +        exit(1);
> +    }
> +
>       snprintf(path, sizeof (path), "/local/domain/0/device-model/%u/state", xen_domid);
> -    if (!xs_write(s->xenstore, XBT_NULL, path, state, strlen(state))) {
> +    if (!xs_write(xs, XBT_NULL, path, state, strlen(state))) {
>           fprintf(stderr, "error recording dm state\n");
>           exit(1);
>       }
> @@ -823,15 +828,20 @@ static void xen_main_loop_prepare(XenIOState *state)
>       if (evtchn_fd != -1) {
>           qemu_set_fd_handler(evtchn_fd, cpu_handle_ioreq, NULL, state);
>       }
> -
> -    /* record state running */
> -    xenstore_record_dm_state(state, "running");
>   }
>
>
>   /* Initialise Xen */
>
> -static void xen_vm_change_state_handler(void *opaque, int running, int reason)
> +static void xen_change_state_handler(void *opaque, int running, int reason)
> +{
> +    if (running) {
> +        /* record state running */
> +        xenstore_record_dm_state(xenstore, "running");
> +    }

So where is this ever executed in the PV case?

Alex
Alexander Graf - June 30, 2011, 2:35 p.m.
On 06/30/2011 04:37 PM, Stefano Stabellini wrote:
> On Thu, 30 Jun 2011, Alexander Graf wrote:
>> On 06/29/2011 01:16 PM, stefano.stabellini@eu.citrix.com wrote:
>>> From: Anthony PERARD<anthony.perard@citrix.com>
>>>
>>> Remove the call to xenstore_record_dm_state from xen_main_loop_prepare
>>> that is HVM specific.
>>> Add a new vm_change_state_handler shared between xen_pv and xen_hvm
>>> machines to record the VM state to xenstore.
>>>
>>> Signed-off-by: Anthony PERARD<anthony.perard@citrix.com>
>>> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>> ---
>>>    xen-all.c |   25 ++++++++++++++++++-------
>>>    1 files changed, 18 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/xen-all.c b/xen-all.c
>>> index 3fd04ef..e8da35f 100644
>>> --- a/xen-all.c
>>> +++ b/xen-all.c
>>> @@ -797,12 +797,17 @@ void xenstore_store_pv_console_info(int i, CharDriverState *chr)
>>>        }
>>>    }
>>>
>>> -static void xenstore_record_dm_state(XenIOState *s, const char *state)
>>> +static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
>>>    {
>>>        char path[50];
>>>
>>> +    if (xs == NULL) {
>>> +        fprintf(stderr, "xenstore connection not initialized\n");
>>> +        exit(1);
>>> +    }
>>> +
>>>        snprintf(path, sizeof (path), "/local/domain/0/device-model/%u/state", xen_domid);
>>> -    if (!xs_write(s->xenstore, XBT_NULL, path, state, strlen(state))) {
>>> +    if (!xs_write(xs, XBT_NULL, path, state, strlen(state))) {
>>>            fprintf(stderr, "error recording dm state\n");
>>>            exit(1);
>>>        }
>>> @@ -823,15 +828,20 @@ static void xen_main_loop_prepare(XenIOState *state)
>>>        if (evtchn_fd != -1) {
>>>            qemu_set_fd_handler(evtchn_fd, cpu_handle_ioreq, NULL, state);
>>>        }
>>> -
>>> -    /* record state running */
>>> -    xenstore_record_dm_state(state, "running");
>>>    }
>>>
>>>
>>>    /* Initialise Xen */
>>>
>>> -static void xen_vm_change_state_handler(void *opaque, int running, int reason)
>>> +static void xen_change_state_handler(void *opaque, int running, int reason)
>>> +{
>>> +    if (running) {
>>> +        /* record state running */
>>> +        xenstore_record_dm_state(xenstore, "running");
>>> +    }
>> So where is this ever executed in the PV case?
>
> qemu_add_vm_change_state_handler(xen_change_state_handler, NULL) is now
> called by xen_init, that is common between xen_pv and xen_fv machines

Ah, there it is. Sorry, I missed it the first time I read the patch :).


Alex
Stefano Stabellini - June 30, 2011, 2:37 p.m.
On Thu, 30 Jun 2011, Alexander Graf wrote:
> On 06/29/2011 01:16 PM, stefano.stabellini@eu.citrix.com wrote:
> > From: Anthony PERARD<anthony.perard@citrix.com>
> >
> > Remove the call to xenstore_record_dm_state from xen_main_loop_prepare
> > that is HVM specific.
> > Add a new vm_change_state_handler shared between xen_pv and xen_hvm
> > machines to record the VM state to xenstore.
> >
> > Signed-off-by: Anthony PERARD<anthony.perard@citrix.com>
> > Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> > ---
> >   xen-all.c |   25 ++++++++++++++++++-------
> >   1 files changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/xen-all.c b/xen-all.c
> > index 3fd04ef..e8da35f 100644
> > --- a/xen-all.c
> > +++ b/xen-all.c
> > @@ -797,12 +797,17 @@ void xenstore_store_pv_console_info(int i, CharDriverState *chr)
> >       }
> >   }
> >
> > -static void xenstore_record_dm_state(XenIOState *s, const char *state)
> > +static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
> >   {
> >       char path[50];
> >
> > +    if (xs == NULL) {
> > +        fprintf(stderr, "xenstore connection not initialized\n");
> > +        exit(1);
> > +    }
> > +
> >       snprintf(path, sizeof (path), "/local/domain/0/device-model/%u/state", xen_domid);
> > -    if (!xs_write(s->xenstore, XBT_NULL, path, state, strlen(state))) {
> > +    if (!xs_write(xs, XBT_NULL, path, state, strlen(state))) {
> >           fprintf(stderr, "error recording dm state\n");
> >           exit(1);
> >       }
> > @@ -823,15 +828,20 @@ static void xen_main_loop_prepare(XenIOState *state)
> >       if (evtchn_fd != -1) {
> >           qemu_set_fd_handler(evtchn_fd, cpu_handle_ioreq, NULL, state);
> >       }
> > -
> > -    /* record state running */
> > -    xenstore_record_dm_state(state, "running");
> >   }
> >
> >
> >   /* Initialise Xen */
> >
> > -static void xen_vm_change_state_handler(void *opaque, int running, int reason)
> > +static void xen_change_state_handler(void *opaque, int running, int reason)
> > +{
> > +    if (running) {
> > +        /* record state running */
> > +        xenstore_record_dm_state(xenstore, "running");
> > +    }
> 
> So where is this ever executed in the PV case?
 
qemu_add_vm_change_state_handler(xen_change_state_handler, NULL) is now
called by xen_init, that is common between xen_pv and xen_fv machines
Alexander Graf - June 30, 2011, 2:43 p.m.
On 06/29/2011 01:16 PM, stefano.stabellini@eu.citrix.com wrote:
> From: Anthony PERARD<anthony.perard@citrix.com>
>
> Remove the call to xenstore_record_dm_state from xen_main_loop_prepare
> that is HVM specific.
> Add a new vm_change_state_handler shared between xen_pv and xen_hvm
> machines to record the VM state to xenstore.
>
> Signed-off-by: Anthony PERARD<anthony.perard@citrix.com>
> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> ---
>   xen-all.c |   25 ++++++++++++++++++-------
>   1 files changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/xen-all.c b/xen-all.c
> index 3fd04ef..e8da35f 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -797,12 +797,17 @@ void xenstore_store_pv_console_info(int i, CharDriverState *chr)
>       }
>   }
>
> -static void xenstore_record_dm_state(XenIOState *s, const char *state)
> +static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
>   {
>       char path[50];
>
> +    if (xs == NULL) {
> +        fprintf(stderr, "xenstore connection not initialized\n");
> +        exit(1);
> +    }

/studio/tmp/agraf/xen-all.c: In function ‘xenstore_record_dm_state’:
/studio/tmp/agraf/xen-all.c:744: error: ‘xs’ undeclared (first use in 
this function)
/studio/tmp/agraf/xen-all.c:744: error: (Each undeclared identifier is 
reported only once
/studio/tmp/agraf/xen-all.c:744: error: for each function it appears in.)


Alex
Stefano Stabellini - July 15, 2011, 10:32 a.m.
I have just realized that I wrote the reply to this email long ago but I
have never sent it, it was sitting in my Draft mbox.

On Thu, 30 Jun 2011, Alexander Graf wrote:
> On 06/29/2011 01:16 PM, stefano.stabellini@eu.citrix.com wrote:
> > From: Anthony PERARD<anthony.perard@citrix.com>
> >
> > Remove the call to xenstore_record_dm_state from xen_main_loop_prepare
> > that is HVM specific.
> > Add a new vm_change_state_handler shared between xen_pv and xen_hvm
> > machines to record the VM state to xenstore.
> >
> > Signed-off-by: Anthony PERARD<anthony.perard@citrix.com>
> > Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> > ---
> >   xen-all.c |   25 ++++++++++++++++++-------
> >   1 files changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/xen-all.c b/xen-all.c
> > index 3fd04ef..e8da35f 100644
> > --- a/xen-all.c
> > +++ b/xen-all.c
> > @@ -797,12 +797,17 @@ void xenstore_store_pv_console_info(int i, CharDriverState *chr)
> >       }
> >   }
> >
> > -static void xenstore_record_dm_state(XenIOState *s, const char *state)
> > +static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
> >   {
> >       char path[50];
> >
> > +    if (xs == NULL) {
> > +        fprintf(stderr, "xenstore connection not initialized\n");
> > +        exit(1);
> > +    }
> 
> /studio/tmp/agraf/xen-all.c: In function ‘xenstore_record_dm_state’:
> /studio/tmp/agraf/xen-all.c:744: error: ‘xs’ undeclared (first use in 
> this function)
> /studio/tmp/agraf/xen-all.c:744: error: (Each undeclared identifier is 
> reported only once
> /studio/tmp/agraf/xen-all.c:744: error: for each function it appears in.)

I think you had some conflicts applying the patch, I'll post it again
rebased on your xen-next branch.

Patch

diff --git a/xen-all.c b/xen-all.c
index 3fd04ef..e8da35f 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -797,12 +797,17 @@  void xenstore_store_pv_console_info(int i, CharDriverState *chr)
     }
 }
 
-static void xenstore_record_dm_state(XenIOState *s, const char *state)
+static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
 {
     char path[50];
 
+    if (xs == NULL) {
+        fprintf(stderr, "xenstore connection not initialized\n");
+        exit(1);
+    }
+
     snprintf(path, sizeof (path), "/local/domain/0/device-model/%u/state", xen_domid);
-    if (!xs_write(s->xenstore, XBT_NULL, path, state, strlen(state))) {
+    if (!xs_write(xs, XBT_NULL, path, state, strlen(state))) {
         fprintf(stderr, "error recording dm state\n");
         exit(1);
     }
@@ -823,15 +828,20 @@  static void xen_main_loop_prepare(XenIOState *state)
     if (evtchn_fd != -1) {
         qemu_set_fd_handler(evtchn_fd, cpu_handle_ioreq, NULL, state);
     }
-
-    /* record state running */
-    xenstore_record_dm_state(state, "running");
 }
 
 
 /* Initialise Xen */
 
-static void xen_vm_change_state_handler(void *opaque, int running, int reason)
+static void xen_change_state_handler(void *opaque, int running, int reason)
+{
+    if (running) {
+        /* record state running */
+        xenstore_record_dm_state(xenstore, "running");
+    }
+}
+
+static void xen_hvm_change_state_handler(void *opaque, int running, int reason)
 {
     XenIOState *state = opaque;
     if (running) {
@@ -854,6 +864,7 @@  int xen_init(void)
         xen_be_printf(NULL, 0, "can't open xen interface\n");
         return -1;
     }
+    qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
 
     return 0;
 }
@@ -915,7 +926,7 @@  int xen_hvm_init(void)
     qemu_map_cache_init();
     xen_ram_init(ram_size);
 
-    qemu_add_vm_change_state_handler(xen_vm_change_state_handler, state);
+    qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
 
     state->client = xen_cpu_phys_memory_client;
     QLIST_INIT(&state->physmap);