I've sent version 2 of this PATCH. I've changed this one since I saw your comments on gvir_save_to_file_domain. <div class="gmail_quote">On Wed, Jul 11, 2012 at 6:18 PM, Zeeshan Ali (Khattak) <<a href="mailto:zeeshanak@gnome.org" target="_blank">zeeshanak@gnome.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi, Forgot review this one. Mostly its the same things I noticed here as the last one. <div class="im"> On Wed, Jul 11, 2012 at 8:28 AM, Jovanka Gulicoska <<a href="mailto:jovanka.gulicoska@gmail.com">jovanka.gulicoska@gmail.com</a>> wrote: > --- > libvirt-gobject/libvirt-gobject-connection.c | 136 ++++++++++++++++++++++++++ > libvirt-gobject/libvirt-gobject-connection.h | 19 ++++ > libvirt-gobject/libvirt-gobject.sym | 3 + > 3 files changed, 158 insertions(+) > > diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c > index 3a99034..2302328 100644 > --- a/libvirt-gobject/libvirt-gobject-connection.c > +++ b/libvirt-gobject/libvirt-gobject-connection.c > @@ -57,6 +57,7 @@ enum { > VIR_CONNECTION_CLOSED, > VIR_DOMAIN_ADDED, > VIR_DOMAIN_REMOVED, > + VIR_DOMAIN_RESTORED, </div>Same as in previous patch: no additional signal needed. <div class="im"> > LAST_SIGNAL > }; > > @@ -228,6 +229,15 @@ static void gvir_connection_class_init(GVirConnectionClass *klass) > 1, > GVIR_TYPE_DOMAIN); > > + signals[VIR_DOMAIN_RESTORED] = g_signal_new("domain-restored", > + G_OBJECT_CLASS_TYPE(object_class), > + G_SIGNAL_RUN_FIRST, > + G_STRUCT_OFFSET(GVirConnectionClass, domain_restored), > + NULL, NULL, > + g_cclosure_marshal_VOID__OBJECT, > + G_TYPE_NONE, > + 0); > + > g_type_class_add_private(klass, sizeof(GVirConnectionPrivate)); > } > > @@ -1605,3 +1615,129 @@ gvir_connection_get_capabilities_finish(GVirConnection *conn, > > return g_object_ref(caps); > } > + > +/* > + * gvir_connection_domain_restore > + */ > +gboolean gvir_connection_domain_restore(GVirConnection *conn, > + gchar *filename, > + GVirConfigDomain *conf, </div>Now that I think of it, I think 'custom_conf' would be a better name for this parameter. Same for the previous patch. <div class="im"> > + guint flags, > + GError **err) > +{ > + GVirConnectionPrivate *priv; > + gchar *custom_xml; > + int ret; > + > + g_return_val_if_fail(GVIR_IS_CONNECTION(conn), FALSE); > + g_return_val_if_fail(GVIR_CONFIG_IS_DOMAIN (conf), FALSE); > + g_return_val_if_fail((err == NULL) || (*err == NULL), FALSE); > + > + priv = conn->priv; > + custom_xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf)); > + > + g_return_val_if_fail(custom_xml != NULL, FALSE); > + > + if(flags || custom_xml) { </div>Same here: if(flags || custom_xml != NULL) Oh and now I realize that you are checking for the string extracted from the parameter for being NULL. You want to check 'conf' itself to be NULL here and then extract the XML string before using it. You'll wan to remove one of the g_return_val_if_fail above to allow user to pass NULL as conf. Same goes for the previous patch too. <div class="im"> > + ret = virDomainRestoreFlags(priv->conn, filename, custom_xml, flags); > + g_free (custom_xml); > + } > + else { > + ret = virDomainRestore(priv->conn, filename); > + g_free (custom_xml); > + } > + > + if(ret < 0) { > + gvir_set_error_literal(err, GVIR_CONNECTION_ERROR, > + 0, > + "Unable to restore domain"); > + > + return FALSE; > + } > + > + return TRUE; > +} > + > +typedef struct { > + gchar *filename; > + gchar *custom_xml; > + guint flags; > +} DomainRestoreFromFileData; </div>Since we are using essentially the same data as for gvir_connection_domain_save, I think we can simply re-use that: #define DomainRestoreFromFileData DomainSaveFromFileData <div class="im"> > +static void domain_restore_from_file_data_free(DomainRestoreFromFileData *data) > +{ </div>Same goes for this function. <div><div class="h5"> > + g_slice_free(DomainRestoreFromFileData, data); > +} > + > +static void > +gvir_connection_domain_restore_helper(GSimpleAsyncResult *res, > + GObject *object, > + GCancellable *cancellable G_GNUC_UNUSED) > +{ > + GVirConnection *conn = GVIR_CONNECTION(object); > + DomainRestoreFromFileData *data; > + GVirConfigDomain *conf; > + GError *err = NULL; > + > + data = g_simple_async_result_get_op_res_gpointer(res); > + conf = gvir_config_domain_new_from_xml(data->custom_xml, &err); > + > + if(!gvir_connection_domain_restore(conn, data->filename, conf, data->flags, &err)) > + g_simple_async_result_take_error(res, err); > +} > + > +/* > + * Async function of gvir_connection_domain_restore > + */ > +void gvir_connection_domain_restore_async(GVirConnection *conn, > + gchar *filename, > + GVirConfigDomain *conf, > + guint flags, > + GCancellable *cancellable, > + GAsyncReadyCallback callback, > + gpointer user_data) </div></div>Possible coding-style issue here as well. <div><div class="h5"> > +{ > + GSimpleAsyncResult *res; > + DomainRestoreFromFileData *data; > + gchar *custom_xml; > + > + g_return_if_fail(GVIR_IS_CONNECTION(conn)); > + g_return_if_fail(GVIR_CONFIG_IS_DOMAIN (conf)); > + g_return_if_fail((cancellable == NULL) || G_IS_CANCELLABLE(cancellable)); > + > + custom_xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf)); > + > + data = g_slice_new0(DomainRestoreFromFileData); > + data->filename = filename; > + data->custom_xml = custom_xml; > + data->flags = flags; > + > + res = g_simple_async_result_new(G_OBJECT(conn), > + callback, > + user_data, > + gvir_connection_domain_restore_async); > + g_simple_async_result_set_op_res_gpointer(res, data, > + (GDestroyNotify)domain_restore_from_file_data_free); > + > + g_simple_async_result_run_in_thread(res, > + gvir_connection_domain_restore_helper, > + G_PRIORITY_DEFAULT, > + cancellable); > + > + g_object_unref(res); > +} > + > +gboolean gvir_connection_domain_restore_finish(GVirConnection *conn, > + GAsyncResult *result, > + GError **err) > +{ > + g_return_val_if_fail(GVIR_IS_CONNECTION(conn), FALSE); > + g_return_val_if_fail(g_simple_async_result_is_valid(result, G_OBJECT(conn), > + gvir_connection_domain_restore_async), > + FALSE); > + > + if (g_simple_async_result_propagate_error(G_SIMPLE_ASYNC_RESULT(result), err)) > + return FALSE; > + > + return TRUE; > +} > diff --git a/libvirt-gobject/libvirt-gobject-connection.h b/libvirt-gobject/libvirt-gobject-connection.h > index c80eecf..01d5d0b 100644 > --- a/libvirt-gobject/libvirt-gobject-connection.h > +++ b/libvirt-gobject/libvirt-gobject-connection.h > @@ -75,6 +75,8 @@ struct _GVirConnectionClass > void (*domain_added)(GVirConnection *conn, GVirDomain *dom); > void (*domain_removed)(GVirConnection *conn, GVirDomain *dom); > > + void (*domain_restored)(GVirConnection *conn); > + > GVirStream *(*stream_new)(GVirConnection *conn, gpointer handle); > > gpointer padding[20]; > @@ -202,6 +204,23 @@ gvir_connection_get_capabilities_finish(GVirConnection *conn, > GAsyncResult *result, > GError **err); > > +gboolean gvir_connection_domain_restore(GVirConnection *conn, > + gchar *filename, > + GVirConfigDomain *conf, > + guint flags, > + GError **err); > + > +void gvir_connection_domain_restore_async(GVirConnection *conn, > + gchar *filename, > + GVirConfigDomain *conf, > + guint flags, > + GCancellable *cancellable, > + GAsyncReadyCallback callback, > + gpointer user_data); > + > +gboolean gvir_connection_domain_restore_finish(GVirConnection *conn, > + GAsyncResult *result, > + GError **err); > G_END_DECLS > > #endif /* __LIBVIRT_GOBJECT_CONNECTION_H__ */ > diff --git a/libvirt-gobject/libvirt-gobject.sym b/libvirt-gobject/libvirt-gobject.sym > index 8ff9e24..0aaefa2 100644 > --- a/libvirt-gobject/libvirt-gobject.sym > +++ b/libvirt-gobject/libvirt-gobject.sym > @@ -31,6 +31,9 @@ LIBVIRT_GOBJECT_0.0.8 { > gvir_connection_create_storage_pool; > gvir_connection_start_domain; > gvir_connection_get_node_info; > + gvir_connection_domain_restore; > + gvir_connection_domain_restore_async; > + gvir_connection_domain_restore_finish; </div></div>Same warning about tabs. -- Regards, Zeeshan Ali (Khattak) FSF member#5124 </blockquote></div>