On Tue, Jul 21, 2015 at 4:09 PM, Daniel P. Berrange <berrange@xxxxxxxxxx> wrote:
> On Tue, Jul 21, 2015 at 03:56:54PM +0100, Zeeshan Ali (Khattak) wrote:
>> On Tue, Jul 21, 2015 at 3:20 PM, Daniel P. Berrange <berrange@xxxxxxxxxx> wrote:
>> > Previously the use of virDomainOpenGraphicsFD API from libvirt
>> > 1.2.8 was made to be conditionally compiled. Given this past
>> > practice, make use of the virNetworkGetDHCPLeases API
>> > conditional too, rather than requiring newer libvirt.
>> > ---
>> > configure.ac | 6 ++-
>> > .../libvirt-gobject-network-dhcp-lease.c | 50 ++++++++++++++++++++++
>> > libvirt-gobject/libvirt-gobject-network.c | 12 ++++++
>> > 3 files changed, 67 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/configure.ac b/configure.ac
>> > index 26beada..228788e 100644
>> > --- a/configure.ac
>> > +++ b/configure.ac
>> > @@ -9,7 +9,7 @@ AC_CANONICAL_HOST
>> >
>> > AM_SILENT_RULES([yes])
>> >
>> > -LIBVIRT_REQUIRED=1.2.6
>> > +LIBVIRT_REQUIRED=0.10.2
>> > AC_SUBST([LIBVIRT_REQUIRED]) dnl used in the .spec file
>> > GLIB2_REQUIRED=2.36.0
>> > AC_SUBST([GLIB2_REQUIRED]) dnl used in the .spec file
>> > @@ -97,6 +97,10 @@ PKG_CHECK_MODULES(LIBVIRT, libvirt >= $LIBVIRT_REQUIRED)
>> > AC_CHECK_LIB([virt],
>> > [virDomainOpenGraphicsFD],
>> > [AC_DEFINE([HAVE_VIR_DOMAIN_OPEN_GRAPHICS_FD], 1, [Have virDomainOpenGraphicsFD?])])
>> > +# virNetworkGetDHCPLeases was introduced in libvirt 1.2.6
>> > +AC_CHECK_LIB([virt],
>> > + [virNetworkGetDHCPLeases],
>> > + [AC_DEFINE([HAVE_VIR_NETWORK_GET_DHCP_LEASES], 1, [Have virNetworkGetDHCPLeases?])])
>> > enable_tests=no
>> > PKG_CHECK_MODULES(GLIB2, glib-2.0 >= $GLIB2_TEST_REQUIRED,
>> > [enable_tests=yes],
>> > diff --git a/libvirt-gobject/libvirt-gobject-network-dhcp-lease.c b/libvirt-gobject/libvirt-gobject-network-dhcp-lease.c
>> > index 6ac3c14..90a402b 100644
>> > --- a/libvirt-gobject/libvirt-gobject-network-dhcp-lease.c
>> > +++ b/libvirt-gobject/libvirt-gobject-network-dhcp-lease.c
>> > @@ -30,14 +30,20 @@
>> > #include "libvirt-glib/libvirt-glib.h"
>> > #include "libvirt-gobject/libvirt-gobject.h"
>> > #include "libvirt-gobject-compat.h"
>> > +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES
>> > #include "libvirt-gobject/libvirt-gobject-network-dhcp-lease-private.h"
>> > +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */
>> >
>> > #define GVIR_NETWORK_DHCP_LEASE_GET_PRIVATE(obj) \
>> > (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_TYPE_NETWORK_DHCP_LEASE, GVirNetworkDHCPLeasePrivate))
>> >
>> > struct _GVirNetworkDHCPLeasePrivate
>> > {
>> > +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES
>> > virNetworkDHCPLeasePtr handle;
>> > +#else
>> > + void *handle;
>> > +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */
>> > };
>> >
>> > G_DEFINE_TYPE(GVirNetworkDHCPLease, gvir_network_dhcp_lease, G_TYPE_OBJECT);
>> > @@ -75,8 +81,10 @@ static void gvir_network_dhcp_lease_set_property(GObject *object,
>> >
>> > switch (prop_id) {
>> > case PROP_HANDLE:
>> > +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES
>> > if (priv->handle)
>> > virNetworkDHCPLeaseFree(priv->handle);
>> > +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */
>> > priv->handle = g_value_get_pointer(value);
>> > break;
>> >
>> > @@ -89,11 +97,15 @@ static void gvir_network_dhcp_lease_set_property(GObject *object,
>> > static void gvir_network_dhcp_lease_finalize(GObject *object)
>> > {
>> > GVirNetworkDHCPLease *lease = GVIR_NETWORK_DHCP_LEASE(object);
>> > +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES
>> > GVirNetworkDHCPLeasePrivate *priv = lease->priv;
>> > +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */
>> >
>> > g_debug("Finalize GVirNetworkDHCPLease=%p", lease);
>>
>> Why not just move this debug below to avoid adding two #ifdef here?
>
> I want the debug message to be the first thing, so you see the
> debug message in the case that the libvirt API call crashes the
> program.
>
>> > diff --git a/libvirt-gobject/libvirt-gobject-network.c b/libvirt-gobject/libvirt-gobject-network.c
>> > index 45dbb71..a278105 100644
>> > --- a/libvirt-gobject/libvirt-gobject-network.c
>> > +++ b/libvirt-gobject/libvirt-gobject-network.c
>> > @@ -29,7 +29,9 @@
>> > #include "libvirt-glib/libvirt-glib.h"
>> > #include "libvirt-gobject/libvirt-gobject.h"
>> > #include "libvirt-gobject-compat.h"
>> > +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES
>> > #include "libvirt-gobject/libvirt-gobject-network-dhcp-lease-private.h"
>> > +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */
>> >
>> > #define GVIR_NETWORK_GET_PRIVATE(obj) \
>> > (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_TYPE_NETWORK, GVirNetworkPrivate))
>> > @@ -249,14 +251,17 @@ GList *gvir_network_get_dhcp_leases(GVirNetwork *network,
>> > guint flags,
>> > GError **err)
>> > {
>> > +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES
>> > virNetworkDHCPLeasePtr *leases;
>> > GList *ret = NULL;
>> > int num_leases, i;
>> > +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */
>> >
>> > g_return_val_if_fail(GVIR_IS_NETWORK(network), NULL);
>> > g_return_val_if_fail(err == NULL || *err == NULL, NULL);
>> > g_return_val_if_fail(flags == 0, NULL);
>> >
>> > +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES
>>
>> Similarly here. I'd just add on #ifdef. If macro is not defined, just
>> return NULL.
>
> You mean to skip the g_return_val_if_fail calls ?
Ah! Didn't think of that. :)
--
Regards,
Zeeshan Ali (Khattak)
________________________________________
Befriend GNOME: http://www.gnome.org/friends/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list