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
pgpr7wKD64ouA.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
