Re: [Qemu-devel] [PATCH v5 1/5] guest agent: command state class

[Luiz Capitulino <lcapitulino@redhat.com>]

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