Re: [libvirt] [glib PATCH] Add gvir_connection_domain_restore() and gvir_connection_domain_restore_async()

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 <jovanka.gulicoska(a)gmail.com&gt; 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