Patchwork [v5,1/5] guest agent: command state class

login
register
mail settings
Submitter Michael Roth
Date June 14, 2011, 8:06 p.m.
Message ID <1308081985-32394-2-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/100430/
State New
Headers show

Comments

Michael Roth - June 14, 2011, 8:06 p.m.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/guest-agent-command-state.c |   73 +++++++++++++++++++++++++++++++++++++++
 qga/guest-agent-core.h          |   25 +++++++++++++
 2 files changed, 98 insertions(+), 0 deletions(-)
 create mode 100644 qga/guest-agent-command-state.c
 create mode 100644 qga/guest-agent-core.h
Luiz Capitulino - June 16, 2011, 6:29 p.m.
On Tue, 14 Jun 2011 15:06:21 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  qga/guest-agent-command-state.c |   73 +++++++++++++++++++++++++++++++++++++++
>  qga/guest-agent-core.h          |   25 +++++++++++++

It's not possible to build this. I see that the last patch has the Makefile
changes, but what we want is that a patch introducing a .c file also updates
the Makefile, so that the .c file can be built.

>  2 files changed, 98 insertions(+), 0 deletions(-)
>  create mode 100644 qga/guest-agent-command-state.c
>  create mode 100644 qga/guest-agent-core.h
> 
> diff --git a/qga/guest-agent-command-state.c b/qga/guest-agent-command-state.c
> new file mode 100644
> index 0000000..969da23
> --- /dev/null
> +++ b/qga/guest-agent-command-state.c
> @@ -0,0 +1,73 @@
> +/*
> + * QEMU Guest Agent command state interfaces
> + *
> + * Copyright IBM Corp. 2011
> + *
> + * Authors:
> + *  Michael Roth      <mdroth@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include <glib.h>
> +#include "qga/guest-agent-core.h"
> +
> +struct GACommandState {
> +    GSList *groups;
> +};
> +
> +typedef struct GACommandGroup {
> +    void (*init)(void);
> +    void (*cleanup)(void);
> +} GACommandGroup;
> +
> +/* handle init/cleanup for stateful guest commands */
> +
> +void ga_command_state_add(GACommandState *cs,
> +                          void (*init)(void),
> +                          void (*cleanup)(void))
> +{
> +    GACommandGroup *cg = g_malloc0(sizeof(GACommandGroup));

This is linked against qemu-ga only, right? Otherwise I think we should
use qemu_mallocz(). And you probably want to check against NULL.

> +    cg->init = init;
> +    cg->cleanup = cleanup;
> +    cs->groups = g_slist_append(cs->groups, cg);

Not that I'm asking to you change anything here, but we're going to
get a funny mix with QObjects :)

> +}
> +
> +static void ga_command_group_init(gpointer opaque, gpointer unused)
> +{
> +    GACommandGroup *cg = opaque;
> +
> +    g_assert(cg);
> +    if (cg->init) {
> +        cg->init();
> +    }
> +}
> +
> +void ga_command_state_init_all(GACommandState *cs)
> +{
> +    g_assert(cs);
> +    g_slist_foreach(cs->groups, ga_command_group_init, NULL);
> +}
> +
> +static void ga_command_group_cleanup(gpointer opaque, gpointer unused)
> +{
> +    GACommandGroup *cg = opaque;
> +
> +    g_assert(cg);
> +    if (cg->cleanup) {
> +        cg->cleanup();
> +    }
> +}
> +
> +void ga_command_state_cleanup_all(GACommandState *cs)
> +{
> +    g_assert(cs);
> +    g_slist_foreach(cs->groups, ga_command_group_cleanup, NULL);
> +}
> +
> +GACommandState *ga_command_state_new(void)
> +{
> +    GACommandState *cs = g_malloc0(sizeof(GACommandState));
> +    cs->groups = NULL;
> +    return cs;
> +}
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> new file mode 100644
> index 0000000..688f120
> --- /dev/null
> +++ b/qga/guest-agent-core.h
> @@ -0,0 +1,25 @@
> +/*
> + * QEMU Guest Agent core declarations
> + *
> + * Copyright IBM Corp. 2011
> + *
> + * Authors:
> + *  Adam Litke        <aglitke@linux.vnet.ibm.com>
> + *  Michael Roth      <mdroth@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "qapi/qmp-core.h"
> +#include "qemu-common.h"
> +
> +#define QGA_VERSION "1.0"
> +
> +typedef struct GACommandState GACommandState;
> +
> +void ga_command_state_add(GACommandState *cs,
> +                          void (*init)(void),
> +                          void (*cleanup)(void));
> +void ga_command_state_init_all(GACommandState *cs);
> +void ga_command_state_cleanup_all(GACommandState *cs);
> +GACommandState *ga_command_state_new(void);
Michael Roth - June 16, 2011, 6:46 p.m.
On 06/16/2011 01:29 PM, Luiz Capitulino wrote:
> On Tue, 14 Jun 2011 15:06:21 -0500
> Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>>   qga/guest-agent-command-state.c |   73 +++++++++++++++++++++++++++++++++++++++
>>   qga/guest-agent-core.h          |   25 +++++++++++++
>
> It's not possible to build this. I see that the last patch has the Makefile
> changes, but what we want is that a patch introducing a .c file also updates
> the Makefile, so that the .c file can be built.
>

I made it a point to test build each C file as introduced, but I'm not 
sure there's a way of doing incremental builds without modifying the 
Makefile manually anyway, since we'll be missing a main() function until 
qemu-ga.c. Or maybe I'm just missing something...any ideas?

>>   2 files changed, 98 insertions(+), 0 deletions(-)
>>   create mode 100644 qga/guest-agent-command-state.c
>>   create mode 100644 qga/guest-agent-core.h
>>
>> diff --git a/qga/guest-agent-command-state.c b/qga/guest-agent-command-state.c
>> new file mode 100644
>> index 0000000..969da23
>> --- /dev/null
>> +++ b/qga/guest-agent-command-state.c
>> @@ -0,0 +1,73 @@
>> +/*
>> + * QEMU Guest Agent command state interfaces
>> + *
>> + * Copyright IBM Corp. 2011
>> + *
>> + * Authors:
>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +#include<glib.h>
>> +#include "qga/guest-agent-core.h"
>> +
>> +struct GACommandState {
>> +    GSList *groups;
>> +};
>> +
>> +typedef struct GACommandGroup {
>> +    void (*init)(void);
>> +    void (*cleanup)(void);
>> +} GACommandGroup;
>> +
>> +/* handle init/cleanup for stateful guest commands */
>> +
>> +void ga_command_state_add(GACommandState *cs,
>> +                          void (*init)(void),
>> +                          void (*cleanup)(void))
>> +{
>> +    GACommandGroup *cg = g_malloc0(sizeof(GACommandGroup));
>
> This is linked against qemu-ga only, right? Otherwise I think we should
> use qemu_mallocz(). And you probably want to check against NULL.
>
>> +    cg->init = init;
>> +    cg->cleanup = cleanup;
>> +    cs->groups = g_slist_append(cs->groups, cg);
>
> Not that I'm asking to you change anything here, but we're going to
> get a funny mix with QObjects :)
>

glib is a hard dependency here so i tried to stick with it where 
possible, since there's a lot of code where i'm kinda forced to use 
GObject stuff, like all the GIO functions for instance (GObject + GError 
+ g_free etc). I think fsfreeze still uses qlist/qemu_free/etc, best I 
can do is fix that up to relegate all the non-glib stuff to qemu-ga.c 
and the qapi/qmp stuff it pulls in.

>> +}
>> +
>> +static void ga_command_group_init(gpointer opaque, gpointer unused)
>> +{
>> +    GACommandGroup *cg = opaque;
>> +
>> +    g_assert(cg);
>> +    if (cg->init) {
>> +        cg->init();
>> +    }
>> +}
>> +
>> +void ga_command_state_init_all(GACommandState *cs)
>> +{
>> +    g_assert(cs);
>> +    g_slist_foreach(cs->groups, ga_command_group_init, NULL);
>> +}
>> +
>> +static void ga_command_group_cleanup(gpointer opaque, gpointer unused)
>> +{
>> +    GACommandGroup *cg = opaque;
>> +
>> +    g_assert(cg);
>> +    if (cg->cleanup) {
>> +        cg->cleanup();
>> +    }
>> +}
>> +
>> +void ga_command_state_cleanup_all(GACommandState *cs)
>> +{
>> +    g_assert(cs);
>> +    g_slist_foreach(cs->groups, ga_command_group_cleanup, NULL);
>> +}
>> +
>> +GACommandState *ga_command_state_new(void)
>> +{
>> +    GACommandState *cs = g_malloc0(sizeof(GACommandState));
>> +    cs->groups = NULL;
>> +    return cs;
>> +}
>> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
>> new file mode 100644
>> index 0000000..688f120
>> --- /dev/null
>> +++ b/qga/guest-agent-core.h
>> @@ -0,0 +1,25 @@
>> +/*
>> + * QEMU Guest Agent core declarations
>> + *
>> + * Copyright IBM Corp. 2011
>> + *
>> + * Authors:
>> + *  Adam Litke<aglitke@linux.vnet.ibm.com>
>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +#include "qapi/qmp-core.h"
>> +#include "qemu-common.h"
>> +
>> +#define QGA_VERSION "1.0"
>> +
>> +typedef struct GACommandState GACommandState;
>> +
>> +void ga_command_state_add(GACommandState *cs,
>> +                          void (*init)(void),
>> +                          void (*cleanup)(void));
>> +void ga_command_state_init_all(GACommandState *cs);
>> +void ga_command_state_cleanup_all(GACommandState *cs);
>> +GACommandState *ga_command_state_new(void);
>
Luiz Capitulino - June 16, 2011, 7:04 p.m.
On Thu, 16 Jun 2011 13:46:31 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 06/16/2011 01:29 PM, Luiz Capitulino wrote:
> > On Tue, 14 Jun 2011 15:06:21 -0500
> > Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
> >
> >>
> >> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> >> ---
> >>   qga/guest-agent-command-state.c |   73 +++++++++++++++++++++++++++++++++++++++
> >>   qga/guest-agent-core.h          |   25 +++++++++++++
> >
> > It's not possible to build this. I see that the last patch has the Makefile
> > changes, but what we want is that a patch introducing a .c file also updates
> > the Makefile, so that the .c file can be built.
> >
> 
> I made it a point to test build each C file as introduced, but I'm not 
> sure there's a way of doing incremental builds without modifying the 
> Makefile manually anyway, since we'll be missing a main() function until 
> qemu-ga.c. Or maybe I'm just missing something...any ideas?

In mind it doesn't make much sense to add a .c file that can't be built, so
you have two options: you merge this in the next patch or you build the object
file only. I like the former more.

> 
> >>   2 files changed, 98 insertions(+), 0 deletions(-)
> >>   create mode 100644 qga/guest-agent-command-state.c
> >>   create mode 100644 qga/guest-agent-core.h
> >>
> >> diff --git a/qga/guest-agent-command-state.c b/qga/guest-agent-command-state.c
> >> new file mode 100644
> >> index 0000000..969da23
> >> --- /dev/null
> >> +++ b/qga/guest-agent-command-state.c
> >> @@ -0,0 +1,73 @@
> >> +/*
> >> + * QEMU Guest Agent command state interfaces
> >> + *
> >> + * Copyright IBM Corp. 2011
> >> + *
> >> + * Authors:
> >> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +#include<glib.h>
> >> +#include "qga/guest-agent-core.h"
> >> +
> >> +struct GACommandState {
> >> +    GSList *groups;
> >> +};
> >> +
> >> +typedef struct GACommandGroup {
> >> +    void (*init)(void);
> >> +    void (*cleanup)(void);
> >> +} GACommandGroup;
> >> +
> >> +/* handle init/cleanup for stateful guest commands */
> >> +
> >> +void ga_command_state_add(GACommandState *cs,
> >> +                          void (*init)(void),
> >> +                          void (*cleanup)(void))
> >> +{
> >> +    GACommandGroup *cg = g_malloc0(sizeof(GACommandGroup));
> >
> > This is linked against qemu-ga only, right? Otherwise I think we should
> > use qemu_mallocz(). And you probably want to check against NULL.
> >
> >> +    cg->init = init;
> >> +    cg->cleanup = cleanup;
> >> +    cs->groups = g_slist_append(cs->groups, cg);
> >
> > Not that I'm asking to you change anything here, but we're going to
> > get a funny mix with QObjects :)
> >
> 
> glib is a hard dependency here so i tried to stick with it where 
> possible, since there's a lot of code where i'm kinda forced to use 
> GObject stuff, like all the GIO functions for instance (GObject + GError 
> + g_free etc). I think fsfreeze still uses qlist/qemu_free/etc, best I 
> can do is fix that up to relegate all the non-glib stuff to qemu-ga.c 
> and the qapi/qmp stuff it pulls in.

Honestly, I don't know where to draw the line. I mean, something we should
really avoid is mixing them in a way that it gets confusing and error
prone, like the same function using Error and GError or allocating data
with g_malloc() and freeing it with qemu_free() (I'm not saying I saw that in
this series, it's just an example).

Now, I don't know why we should use g_malloc() for example. Does it have
any additional features? Like what talloc has? If it doesn't have, then I
think we should use qemu's variants.

Another example is glib's list implementation. I think it shouldn't be used
when qemu's QLIST suffices.

> 
> >> +}
> >> +
> >> +static void ga_command_group_init(gpointer opaque, gpointer unused)
> >> +{
> >> +    GACommandGroup *cg = opaque;
> >> +
> >> +    g_assert(cg);
> >> +    if (cg->init) {
> >> +        cg->init();
> >> +    }
> >> +}
> >> +
> >> +void ga_command_state_init_all(GACommandState *cs)
> >> +{
> >> +    g_assert(cs);
> >> +    g_slist_foreach(cs->groups, ga_command_group_init, NULL);
> >> +}
> >> +
> >> +static void ga_command_group_cleanup(gpointer opaque, gpointer unused)
> >> +{
> >> +    GACommandGroup *cg = opaque;
> >> +
> >> +    g_assert(cg);
> >> +    if (cg->cleanup) {
> >> +        cg->cleanup();
> >> +    }
> >> +}
> >> +
> >> +void ga_command_state_cleanup_all(GACommandState *cs)
> >> +{
> >> +    g_assert(cs);
> >> +    g_slist_foreach(cs->groups, ga_command_group_cleanup, NULL);
> >> +}
> >> +
> >> +GACommandState *ga_command_state_new(void)
> >> +{
> >> +    GACommandState *cs = g_malloc0(sizeof(GACommandState));
> >> +    cs->groups = NULL;
> >> +    return cs;
> >> +}
> >> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> >> new file mode 100644
> >> index 0000000..688f120
> >> --- /dev/null
> >> +++ b/qga/guest-agent-core.h
> >> @@ -0,0 +1,25 @@
> >> +/*
> >> + * QEMU Guest Agent core declarations
> >> + *
> >> + * Copyright IBM Corp. 2011
> >> + *
> >> + * Authors:
> >> + *  Adam Litke<aglitke@linux.vnet.ibm.com>
> >> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +#include "qapi/qmp-core.h"
> >> +#include "qemu-common.h"
> >> +
> >> +#define QGA_VERSION "1.0"
> >> +
> >> +typedef struct GACommandState GACommandState;
> >> +
> >> +void ga_command_state_add(GACommandState *cs,
> >> +                          void (*init)(void),
> >> +                          void (*cleanup)(void));
> >> +void ga_command_state_init_all(GACommandState *cs);
> >> +void ga_command_state_cleanup_all(GACommandState *cs);
> >> +GACommandState *ga_command_state_new(void);
> >
>

Patch

diff --git a/qga/guest-agent-command-state.c b/qga/guest-agent-command-state.c
new file mode 100644
index 0000000..969da23
--- /dev/null
+++ b/qga/guest-agent-command-state.c
@@ -0,0 +1,73 @@ 
+/*
+ * QEMU Guest Agent command state interfaces
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include <glib.h>
+#include "qga/guest-agent-core.h"
+
+struct GACommandState {
+    GSList *groups;
+};
+
+typedef struct GACommandGroup {
+    void (*init)(void);
+    void (*cleanup)(void);
+} GACommandGroup;
+
+/* handle init/cleanup for stateful guest commands */
+
+void ga_command_state_add(GACommandState *cs,
+                          void (*init)(void),
+                          void (*cleanup)(void))
+{
+    GACommandGroup *cg = g_malloc0(sizeof(GACommandGroup));
+    cg->init = init;
+    cg->cleanup = cleanup;
+    cs->groups = g_slist_append(cs->groups, cg);
+}
+
+static void ga_command_group_init(gpointer opaque, gpointer unused)
+{
+    GACommandGroup *cg = opaque;
+
+    g_assert(cg);
+    if (cg->init) {
+        cg->init();
+    }
+}
+
+void ga_command_state_init_all(GACommandState *cs)
+{
+    g_assert(cs);
+    g_slist_foreach(cs->groups, ga_command_group_init, NULL);
+}
+
+static void ga_command_group_cleanup(gpointer opaque, gpointer unused)
+{
+    GACommandGroup *cg = opaque;
+
+    g_assert(cg);
+    if (cg->cleanup) {
+        cg->cleanup();
+    }
+}
+
+void ga_command_state_cleanup_all(GACommandState *cs)
+{
+    g_assert(cs);
+    g_slist_foreach(cs->groups, ga_command_group_cleanup, NULL);
+}
+
+GACommandState *ga_command_state_new(void)
+{
+    GACommandState *cs = g_malloc0(sizeof(GACommandState));
+    cs->groups = NULL;
+    return cs;
+}
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
new file mode 100644
index 0000000..688f120
--- /dev/null
+++ b/qga/guest-agent-core.h
@@ -0,0 +1,25 @@ 
+/*
+ * QEMU Guest Agent core declarations
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Adam Litke        <aglitke@linux.vnet.ibm.com>
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "qapi/qmp-core.h"
+#include "qemu-common.h"
+
+#define QGA_VERSION "1.0"
+
+typedef struct GACommandState GACommandState;
+
+void ga_command_state_add(GACommandState *cs,
+                          void (*init)(void),
+                          void (*cleanup)(void));
+void ga_command_state_init_all(GACommandState *cs);
+void ga_command_state_cleanup_all(GACommandState *cs);
+GACommandState *ga_command_state_new(void);