I've sent version 2 of this PATCH. I've changed this one since I saw your comments on gvir_save_to_file_domain.
On Wed, Jul 11, 2012 at 6:18 PM, Zeeshan Ali (Khattak) <[email protected]>wrote: > Hi, > Forgot review this one. Mostly its the same things I noticed here > as the last one. > > On Wed, Jul 11, 2012 at 8:28 AM, Jovanka Gulicoska > <[email protected]> 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, > > Same as in previous patch: no additional signal needed. > > > 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, > > Now that I think of it, I think 'custom_conf' would be a better name > for this parameter. Same for the previous patch. > > > + 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) { > > 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. > > > + 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; > > 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 > > > +static void > domain_restore_from_file_data_free(DomainRestoreFromFileData *data) > > +{ > > Same goes for this function. > > > + 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) > > Possible coding-style issue here as well. > > > +{ > > + 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; > > Same warning about tabs. > > -- > Regards, > > Zeeshan Ali (Khattak) > FSF member#5124 >
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
