[libvirt] [libvirt-glib 04/37] Add some GVirConfigClock setters

[Christophe Fergeau]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111115/d173b763/attachment-0001.sig>