On Thu, 16 Jun 2011 13:46:31 -0500
Michael Roth <mdr...@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<mdr...@linux.vnet.ibm.com> wrote:
> >
> >>
> >> Signed-off-by: Michael Roth<mdr...@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<mdr...@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<agli...@linux.vnet.ibm.com>
> >> + * Michael Roth<mdr...@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);
> >
>
