On Fri, Nov 11, 2011 at 03:39:01PM +0100, Marc-André Lureau wrote:
+void gvir_config_clock_set_timezone(GVirConfigClock *klock, + const char *tz) +{ + xmlNodePtr node; + xmlChar *encoded_tz;
in general, I think we should add g_return_..if_fail() for all public functions, and all arguments. Would you mind adding it in your patches?
Hmm ok So g_return_if_fail(klock != NULL); g_return_if_fail(tz != NULL); ? (just want to be 100% sure we are talking about the same thing :)
+ + node = gvir_config_object_get_xml_node(GVIR_CONFIG_OBJECT(klock)); + if (node == NULL) + return; + + xmlNewProp(node, (xmlChar*)"offset", (xmlChar*)"timezone"); + encoded_tz = xmlEncodeEntitiesReentrant(node->doc, (xmlChar*)tz); + xmlNewProp(node, (xmlChar*)"timezone", encoded_tz); + xmlFree(encoded_tz);
the (xmlChar*) and (gchar*) casts are very frequent in the code, and make the code harder to read. I wonder if we should have macro such as XCHAR() GCHAR(). I think I have since that in other projects.
libxml2 provides http://xmlsoft.org/html/libxml-xmlstring.html#BAD_CAST which isn't really easy on the eyes. I've got more patches which are not ready for review yet, but which move these casts in only a few helper functions instead of having them everywhere. If you don't mind, I'll address this when all these patches have been sent, is it ok?
diff --git a/libvirt-gconfig/libvirt-gconfig-clock.h b/libvirt-gconfig/libvirt-gconfig-clock.h index fc8850a..26f4b53 100644 --- a/libvirt-gconfig/libvirt-gconfig-clock.h +++ b/libvirt-gconfig/libvirt-gconfig-clock.h @@ -63,6 +63,11 @@ GVirConfigClock *gvir_config_clock_new(void); GVirConfigClock *gvir_config_clock_new_from_xml(const gchar *xml, GError **error);
+void gvir_config_clock_set_timezone(GVirConfigClock *klock, + const char *tz); +void gvir_config_clock_set_variable_offset(GVirConfigClock *klock, + gint seconds); + G_END_DECLS
#endif /* __LIBVIRT_GCONFIG_CLOCK_H__ */ diff --git a/libvirt-gconfig/libvirt-gconfig-domain.c b/libvirt-gconfig/libvirt-gconfig-domain.c index c847c14..f80720a 100644 --- a/libvirt-gconfig/libvirt-gconfig-domain.c +++ b/libvirt-gconfig/libvirt-gconfig-domain.c @@ -236,3 +236,4 @@ void gvir_config_domain_set_features(GVirConfigDomain *domain, } g_object_notify(G_OBJECT(domain), "features"); } +
Extra line?
Removed, though I think it goes away in one of the next commits
diff --git a/libvirt-gconfig/libvirt-gconfig-domain.h b/libvirt-gconfig/libvirt-gconfig-domain.h index d9f0c09..6cc8f31 100644 --- a/libvirt-gconfig/libvirt-gconfig-domain.h +++ b/libvirt-gconfig/libvirt-gconfig-domain.h @@ -70,7 +70,6 @@ GStrv gvir_config_domain_get_features(GVirConfigDomain *domain); void gvir_config_domain_set_features(GVirConfigDomain *domain, const GStrv features);
-
ok :)
I've moved this to another commit modifying set_features. The commit introducing it has been pushed already unfortunately. Christophe