[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [glib PATCH] Add givr_domain_save_to_file() and gvir_domain_save_to_file_async



Hi Jovanka,
   Starting to look really good already. Some comments below:

On Wed, Jul 11, 2012 at 8:26 AM, Jovanka Gulicoska
<jovanka gulicoska gmail com> wrote:
> ---
>  libvirt-gobject/libvirt-gobject-domain.c |  138 ++++++++++++++++++++++++++++++
>  libvirt-gobject/libvirt-gobject-domain.h |   19 ++++
>  libvirt-gobject/libvirt-gobject.sym      |    3 +
>  3 files changed, 160 insertions(+)
>
> diff --git a/libvirt-gobject/libvirt-gobject-domain.c b/libvirt-gobject/libvirt-gobject-domain.c
> index 088cd33..356b15b 100644
> --- a/libvirt-gobject/libvirt-gobject-domain.c
> +++ b/libvirt-gobject/libvirt-gobject-domain.c
> @@ -55,6 +55,7 @@ enum {
>      VIR_RESUMED,
>      VIR_STOPPED,
>      VIR_UPDATED,
> +    VIR_SAVED_TO_FILE,

There is no need for adding this signal. We already have appropriate
signals for the relevant libvirt events ("stopped::saved" in this
case). Besides you are not emitting this signal.

>      LAST_SIGNAL
>  };
>
> @@ -225,6 +226,16 @@ static void gvir_domain_class_init(GVirDomainClass *klass)
>                                          G_TYPE_NONE,
>                                          0);
>
> +     signals[VIR_SAVED_TO_FILE] = g_signal_new("saved_to_file",
> +                                              G_OBJECT_CLASS_TYPE(object_class),
> +                                              G_SIGNAL_RUN_LAST | G_SIGNAL_NO_RECURSE |
> +                                              G_SIGNAL_NO_HOOKS | G_SIGNAL_DETAILED,
> +                                              G_STRUCT_OFFSET(GVirDomainClass, saved_to_file),
> +                                              NULL, NULL,
> +                                              g_cclosure_marshal_VOID__VOID,
> +                                              G_TYPE_NONE,
> +                                              0);
> +
>      g_type_class_add_private(klass, sizeof(GVirDomainPrivate));
>  }
>
> @@ -556,6 +567,133 @@ gboolean gvir_domain_reboot(GVirDomain *dom,
>      return TRUE;
>  }
>
> +gboolean gvir_domain_save_to_file_config(GVirDomain *dom,

No need for the additional '_config' suffix.

> +                                         gchar *filename,
> +                                         GVirConfigDomain *conf,
> +                                         guint flags,
> +                                         GError **err)
> +{
> +    GVirDomainPrivate *priv;
> +    gchar *custom_xml;
> +    int ret;
> +
> +    g_return_val_if_fail(GVIR_IS_DOMAIN(dom), FALSE);
> +    g_return_val_if_fail(GVIR_CONFIG_IS_DOMAIN (conf), FALSE);
> +    g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
> +
> +    priv = dom->priv;
> +    custom_xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf));
> +
> +    if (flags || custom_xml) {

I'd prefer explicit NULL check (compiler can optimize for us):

 if (flags || custom_xml != NULL) {

> +        ret = virDomainSaveFlags(priv->handle, filename, custom_xml, flags);
> +        g_free (custom_xml);
> +    }
> +    else {
> +        ret = virDomainSave(priv->handle, filename);
> +        g_free (custom_xml);
> +    }
> +    if (ret < 0) {
> +        gvir_set_error_literal(err, GVIR_DOMAIN_ERROR,
> +                                 0,
> +                                 "Unable to save domain to file");
> +        return FALSE;
> +    }
> +
> +    return TRUE;
> +}
> +
> +typedef struct {
> +    gchar *filename;
> +    gchar *custom_xml;
> +    guint flags;
> +} DomainSaveToFileData;
> +
> +static void domain_save_to_file_data_free(DomainSaveToFileData *data)
> +{

You need to duplicate (g_strdup) the 'custom_xml' and 'filename'
strings when you put them in the data and then free them here.

> +    g_slice_free(DomainSaveToFileData, data);
> +}
> +
> +static void
> +gvir_domain_save_to_file_helper(GSimpleAsyncResult *res,
> +                                GObject *object,
> +                                GCancellable *cancellable G_GNUC_UNUSED)

Its hard to be sure with this gmail's web interface but I think last 2
arguments aren't aligned. Please do read the HACKING file and read
through your code/patches to see if it follows the conventions.

> +{
> +    GVirDomain *dom = GVIR_DOMAIN(object);
> +    DomainSaveToFileData *data;
> +    GVirConfigDomain *conf;
> +    GError *err = NULL;
> +  //  gchar *xml;

Some redundant commented-out code here and below. I guess you forgot to remove.

> +    data = g_simple_async_result_get_op_res_gpointer(res);
> +  //      //    xml = data->filename;
> +    conf = gvir_domain_get_config(dom, data->flags, &err);
> +
> +    if (!gvir_domain_save_to_file_config(dom, data->filename, conf, data->flags, &err))
> +                       g_simple_async_result_take_error(res, err);
> + }
> +
> +/**
> + * gvir_domain_save_to_file_async:
> + * @dom: the domain
> + * @flags: the flags
> + * @cancellable: cancallation object
> + * @callback: (scope async): completion callback
> + * @user_data: (closure): opaque data for callback
> + *
> + * Asynchronous variant of #gvir_domain_save_to_file.
> + */
> +
> +void gvir_domain_save_to_file_async (GVirDomain *dom,
> +                                     gchar *filename,
> +                                     GVirConfigDomain *conf,
> +                                     guint flags,
> +                                     GCancellable *cancellable,
> +                                     GAsyncReadyCallback callback,
> +                                     gpointer user_data)
> +{
> +    GSimpleAsyncResult *res;
> +    DomainSaveToFileData *data;
> +    gchar *xml;
> +
> +    g_return_if_fail(GVIR_IS_DOMAIN(dom));
> +    g_return_if_fail(GVIR_CONFIG_IS_DOMAIN (conf));
> +    g_return_if_fail((cancellable == NULL) || G_IS_CANCELLABLE(cancellable));
> +
> +    xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf));
> +
> +    data = g_slice_new0(DomainSaveToFileData);
> +    data->filename = filename;
> +    data->custom_xml = xml;
> +    data->flags = flags;
> +
> +    res = g_simple_async_result_new(G_OBJECT(dom),
> +                                    callback,
> +                                    user_data,
> +                                    gvir_domain_save_to_file_async);
> +    g_simple_async_result_set_op_res_gpointer(res, data, (GDestroyNotify)domain_save_to_file_data_free);
> +
> +    g_simple_async_result_run_in_thread(res,
> +                                        gvir_domain_save_to_file_helper,
> +                                        G_PRIORITY_DEFAULT,
> +                                        cancellable);
> +
> +    g_object_unref(res);
> +}
> +
> +gboolean gvir_domain_save_to_file_finish(GVirDomain *dom,
> +                                         GAsyncResult *result,
> +                                         GError **err)
> +{
> +    g_return_val_if_fail(GVIR_IS_DOMAIN(dom), FALSE);
> +    g_return_val_if_fail(g_simple_async_result_is_valid(result, G_OBJECT(dom), gvir_domain_save_to_file_async), FALSE);
> +    g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
> +
> +    if (g_simple_async_result_propagate_error(G_SIMPLE_ASYNC_RESULT(result), err))
> +        return FALSE;
> +
> +    return TRUE;
> +}
> +
>  /**
>   * gvir_domain_get_config:
>   * @dom: the domain
> diff --git a/libvirt-gobject/libvirt-gobject-domain.h b/libvirt-gobject/libvirt-gobject-domain.h
> index 87b94f4..1df54de 100644
> --- a/libvirt-gobject/libvirt-gobject-domain.h
> +++ b/libvirt-gobject/libvirt-gobject-domain.h
> @@ -65,6 +65,7 @@ struct _GVirDomainClass
>      void (*resumed)(GVirDomain *dom);
>      void (*updated)(GVirDomain *dom);
>      void (*suspended)(GVirDomain *dom);
> +    void (*saved_to_file) (GVirDomain *dom);

This will also go away with the signal.

>      gpointer padding[20];
>  };
> @@ -148,6 +149,24 @@ gboolean gvir_domain_reboot(GVirDomain *dom,
>                              guint flags,
>                              GError **err);
>
> +void gvir_domain_save_to_file_async (GVirDomain *dom,
> +                                     gchar *filename,
> +                                     GVirConfigDomain *conf,
> +                                     guint flags,
> +                                     GCancellable *cancellable,
> +                                     GAsyncReadyCallback callback,
> +                                     gpointer user_data);
> +
> +gboolean gvir_domain_save_to_file_finish(GVirDomain *dom,
> +                                  GAsyncResult *result,
> +                                  GError **err);
> +
> +gboolean gvir_domain_save_to_file_config(GVirDomain *dom,
> +                                         gchar *filename,
> +                                         GVirConfigDomain *conf,
> +                                         guint flags,
> +                                         GError **err);
> +
>  GVirDomainInfo *gvir_domain_get_info(GVirDomain *dom,
>                                       GError **err);
>  void gvir_domain_get_info_async(GVirDomain *dom,
> diff --git a/libvirt-gobject/libvirt-gobject.sym b/libvirt-gobject/libvirt-gobject.sym
> index 94e441a..8ff9e24 100644
> --- a/libvirt-gobject/libvirt-gobject.sym
> +++ b/libvirt-gobject/libvirt-gobject.sym
> @@ -75,6 +75,9 @@ LIBVIRT_GOBJECT_0.0.8 {
>         gvir_domain_get_persistent;
>         gvir_domain_get_saved;
>         gvir_domain_screenshot;
> +        gvir_domain_save_to_file_config;
> +        gvir_domain_save_to_file_async;
> +        gvir_domain_save_to_file_finish;

Beware that this file (unlike the rest of the code) uses tabs rather
than spaces for indenting.

BTW, commit log summary could just be: Add bindings for virDomainSave*().

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]