On Tue, Oct 22, 2013 at 09:32:58AM +0100, Daniel P. Berrange wrote:
> On Wed, Oct 16, 2013 at 11:12:46AM +0200, Christophe Fergeau wrote:
> > On Tue, Oct 15, 2013 at 12:05:02PM -0700, Ian Main wrote:
> > > This patch adds support for setting filterref's on interfaces. Also
> > > supported are parameters to the filterref's.
> >
> > This mostly looks good, some comments below.
> >
>
> > > +GVirConfigDomainInterfaceFilterref *gvir_config_domain_interface_filterref_new_from_xml(const gchar *xml,
> > > + GError **error)
> > > +{
> > > + GVirConfigObject *object;
> > > +
> > > + object = gvir_config_object_new_from_xml(GVIR_CONFIG_TYPE_DOMAIN_INTERFACE_FILTERREF,
> > > + "filterref", NULL, xml, error);
> > > + if (g_strcmp0(gvir_config_object_get_attribute(object, NULL, "type"), "filterref") != 0) {
> >
> > Is this test correct? I don't see a 'type' attribute on 'filterref' nodes
> > in http://libvirt.org/formatnwfilter.html
> >
> > > + g_object_unref(G_OBJECT(object));
> > > + return NULL;
> > > + }
> > > + return GVIR_CONFIG_DOMAIN_INTERFACE_FILTERREF(object);
> > > +}
> > > +
> > > +void gvir_config_domain_interface_filterref_set_filter(GVirConfigDomainInterfaceFilterref *filterref,
> > > + const char *filter)
> >
> > I'm wondering if we should call that method
> > gvir_config_domain_interface_filterref_set_filter_name()
>
> I'd go one step simpler and call it
>
> gvir_config_domain_interface_filterref_set_name()
It's funny because this is what I had originally.. I renamed it as I
thought it would be better to follow the XML more precisely. That way
people that know the XML well won't be confused when using the API.
That was my thinking anyway. However, I'll post a patch with it changed.
Do we want to update the libvirt-sandbox patches to use 'name' as well?
Ian
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list