Hi Jovanka,
Starting to look really good already. Some comments below:
On Wed, Jul 11, 2012 at 8:26 AM, Jovanka Gulicoska
<jovanka.gulicoska@xxxxxxxxx> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list