[libvirt] [glib PATCH V3] Add bindings for virDomainSave*()

[Zeeshan Ali (Khattak)]

Hi,
  Its getting into a mergable state soon. Some comments/issues still:

Firstly there seems to be trailing whitespaces in this patch. Please
remove those. Haven't yet checked the other patches but please make
sure they don't have those either. More comments inlined:

On Wed, Jul 11, 2012 at 11:42 PM, Jovanka Gulicoska
<jovanka.gulicoska at gmail.com> wrote:
> ---
>  libvirt-gobject/libvirt-gobject-domain.c |  145 ++++++++++++++++++++++++++++++
>  libvirt-gobject/libvirt-gobject-domain.h |   18 ++++
>  libvirt-gobject/libvirt-gobject.sym      |    3 +
>  3 files changed, 166 insertions(+)
>
> diff --git a/libvirt-gobject/libvirt-gobject-domain.c b/libvirt-gobject/libvirt-gobject-domain.c
> index 088cd33..d9688f7 100644
> --- a/libvirt-gobject/libvirt-gobject-domain.c
> +++ b/libvirt-gobject/libvirt-gobject-domain.c
> @@ -557,6 +557,151 @@ gboolean gvir_domain_reboot(GVirDomain *dom,
>  }
>
>  /**
> + * gvir_domain_save_to_file:
> + * @dom: the domain
> + * @filename: path to the output file
> + * @custom_conf: configuration for domain or NULL
> + * @flags: the flags
> + *
> + * Returns: TRUE on success, FALSE otherwise
> + */
> +gboolean gvir_domain_save_to_file(GVirDomain *dom,
> +                                  gchar *filename,
> +                                  GVirConfigDomain *custom_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(err == NULL || *err == NULL, FALSE);
> +
> +    priv = dom->priv;
> +
> +    if (flags || (custom_conf != NULL)) {

* No need for '()' around the NULL check.
* You need to do this check again before trying to get xml out of the
object as this condition could be TRUE just because flag was non-zero.
Something like:

    char *custom_xml = NULL;

    if (custom_conf != NULL)
        custom_xml == ..

> +        custom_xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(custom_conf));
> +        ret = virDomainSaveFlags(priv->handle, filename, custom_xml, flags);
> +        g_free (custom_xml);
> +    }
> +    else
> +        ret = virDomainSave(priv->handle, filename);

Quoting directly from HACKING file:

 - If a brace needs to be used for one clause in an if/else statement,
   it should be used for both clauses, even if the other clauses are
   only single statements. eg

      if (foo) {
         bar;
         wizz;
      } else {
         eek;
      }

   Not

      if (foo) {
         bar;
         wizz;
      } else
         eek;

> +    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)
> +{
> +    g_free(data->filename);
> +    g_free(data->custom_xml);
> +    g_slice_free(DomainSaveToFileData, data);
> +}
> +
> +static void
> +gvir_domain_save_to_file_helper(GSimpleAsyncResult *res,
> +                                GObject *object,
> +                                GCancellable *cancellable G_GNUC_UNUSED)
> +{
> +    GVirDomain *dom = GVIR_DOMAIN(object);
> +    DomainSaveToFileData *data;
> +    GVirConfigDomain *conf;
> +    GError *err = NULL;
> +
> +    data = g_simple_async_result_get_op_res_gpointer(res);
> +    conf = gvir_domain_get_config(dom, data->flags, &err);
> +
> +    if (!gvir_domain_save_to_file(dom, data->filename, conf, data->flags, &err))
> +        g_simple_async_result_take_error(res, err);
> +}
> +
> +/**
> + * gvir_domain_save_to_file_async:
> + * @dom: the domain
> + * @filename: path to output file
> + * @custom_conf: configuration for domain

You want to declare this and cancellable as nullable for GIR using
'allow-none' annotation. Check gvir_domain_start_async's doc comment
to see how. Same goes for any nullable parameters in this and other
patches of yours.

> + * @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 *custom_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 (custom_conf));

You forgot to remove this line.

> +    g_return_if_fail((cancellable == NULL) || G_IS_CANCELLABLE(cancellable));
> +
> +    xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(custom_conf));
> +
> +    data = g_slice_new0(DomainSaveToFileData);
> +    data->filename = g_strdup(filename);
> +    data->custom_xml = g_strdup(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);
> +}
> +
> +/**
> + * gvir_domain_save_to_file_finish:
> + * @dom: the domain to save
> + * @result: (transfer none): async method result
> + * @err: Place-holder for possible errors
> + *
> + * Finishes the operation started by #gvir_domain_save_to_file_async.
> + *
> + * Returns: TRUE if domain was saved successfully, FALSE otherwise.
> + */
> +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);

This goes far beyond 80 cols so you want to break the
g_simple_async_result_is_valid to multiple lines.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124