[libvirt] [PATCH glib] Remove dep on libvirt 1.2.6
Going back on what I said a few hours ago.... On closer inspection, I re-discovered that we have already taken the approach of conditionally compiling code in libvirt-gobject to avoid increasing the min libvirt. So given this, I would like to avoid increasing the min required libvirt in the release we're about to do today for benefit of GNOME boxes. I think we can still have a valid discussion about increasing the min libvirt after this release is done, since I think that 0.10.2 is really far too old. It is based on RHEL-6 vintage libvirt and that's not really useful due to lack of gobject introspection. So to clarify, I think we should set a clear policy on what platforms we're going to target going forward. This will let us make an easy & clear decision about when a patch needs to use the conditional compilation approach. Daniel P. Berrange (1): Make use of DHCP API conditionally compiled configure.ac | 6 ++- .../libvirt-gobject-network-dhcp-lease.c | 50 ++++++++++++++++++++++ libvirt-gobject/libvirt-gobject-network.c | 12 ++++++ 3 files changed, 67 insertions(+), 1 deletion(-) -- 2.4.3
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); +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES virNetworkDHCPLeaseFree(priv->handle); +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ G_OBJECT_CLASS(gvir_network_dhcp_lease_parent_class)->finalize(object); } @@ -127,12 +139,14 @@ static void gvir_network_dhcp_lease_init(GVirNetworkDHCPLease *lease) lease->priv = GVIR_NETWORK_DHCP_LEASE_GET_PRIVATE(lease); } +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES GVirNetworkDHCPLease *gvir_network_dhcp_lease_new(virNetworkDHCPLeasePtr handle) { return g_object_new(GVIR_TYPE_NETWORK_DHCP_LEASE, "handle", handle, NULL); } +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ /** * gvir_network_dhcp_lease_get_iface: @@ -144,7 +158,11 @@ const gchar *gvir_network_dhcp_lease_get_iface(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), NULL); +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->iface; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ } /** @@ -157,7 +175,11 @@ gint64 gvir_network_dhcp_lease_get_expiry_time(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), -1); +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->expirytime; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return -1; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ } /** @@ -170,7 +192,11 @@ gint gvir_network_dhcp_lease_get_ip_type(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), -1); +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->type; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return -1; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ } /** @@ -183,7 +209,11 @@ const gchar *gvir_network_dhcp_lease_get_mac(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), NULL); +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->mac; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ } /** @@ -196,7 +226,11 @@ const gchar *gvir_network_dhcp_lease_get_iaid(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), NULL); +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->iaid; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ } /** @@ -209,7 +243,11 @@ const gchar *gvir_network_dhcp_lease_get_ip(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), NULL); +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->ipaddr; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ } /** @@ -222,7 +260,11 @@ guint gvir_network_dhcp_lease_get_prefix(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), 0); +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->prefix; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return 0; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ } /** @@ -235,7 +277,11 @@ const gchar *gvir_network_dhcp_lease_get_hostname(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), NULL); +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->hostname; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ } /** @@ -248,5 +294,9 @@ const gchar *gvir_network_dhcp_lease_get_client_id(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), NULL); +#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->clientid; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ } 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 num_leases = virNetworkGetDHCPLeases(network->priv->handle, mac, &leases, flags); if (num_leases < 0) { gvir_set_error_literal(err, GVIR_NETWORK_ERROR, @@ -277,4 +282,11 @@ GList *gvir_network_get_dhcp_leases(GVirNetwork *network, free(leases); return g_list_reverse(ret); +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + (void)mac; + gvir_set_error_literal(err, GVIR_NETWORK_ERROR, + 0, + "Unable to get network DHCP leases"); + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ } -- 2.4.3
Hi, On Tue, Jul 21, 2015 at 03:20:03PM +0100, Daniel P. Berrange 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.
I've tested compilation on f22 with/without finding virNetworkGetDHCPLeases (I hacked the configure check for the latter). I do not have a RHEL 7.0 handy to test this.
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 num_leases = virNetworkGetDHCPLeases(network->priv->handle, mac, &leases, flags); if (num_leases < 0) { gvir_set_error_literal(err, GVIR_NETWORK_ERROR, @@ -277,4 +282,11 @@ GList *gvir_network_get_dhcp_leases(GVirNetwork *network, free(leases);
return g_list_reverse(ret); +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + (void)mac;
An alternative would be to add a G_GNUC_UNUSED in the argument list as this only indicates that the argument may be unused (as opposed to "is really unused"). Both are fine with me.
+ gvir_set_error_literal(err, GVIR_NETWORK_ERROR, + 0, + "Unable to get network DHCP leases");
This should be g_set_error_literal(). ACK from me otherwise. Christophe
On Tue, Jul 21, 2015 at 04:55:15PM +0200, Christophe Fergeau wrote:
Hi,
On Tue, Jul 21, 2015 at 03:20:03PM +0100, Daniel P. Berrange 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.
I've tested compilation on f22 with/without finding virNetworkGetDHCPLeases (I hacked the configure check for the latter). I do not have a RHEL 7.0 handy to test this.
FYI I built a copy of 0.10.2 and tested against it
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 num_leases = virNetworkGetDHCPLeases(network->priv->handle, mac, &leases, flags); if (num_leases < 0) { gvir_set_error_literal(err, GVIR_NETWORK_ERROR, @@ -277,4 +282,11 @@ GList *gvir_network_get_dhcp_leases(GVirNetwork *network, free(leases);
return g_list_reverse(ret); +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + (void)mac;
An alternative would be to add a G_GNUC_UNUSED in the argument list as this only indicates that the argument may be unused (as opposed to "is really unused"). Both are fine with me.
Yep
+ gvir_set_error_literal(err, GVIR_NETWORK_ERROR, + 0, + "Unable to get network DHCP leases");
This should be g_set_error_literal().
Ah good point.
ACK from me otherwise.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jul 21, 2015 at 3:20 PM, Daniel P. Berrange <berrange@redhat.com> 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?
+#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES virNetworkDHCPLeaseFree(priv->handle); +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */
G_OBJECT_CLASS(gvir_network_dhcp_lease_parent_class)->finalize(object); } @@ -127,12 +139,14 @@ static void gvir_network_dhcp_lease_init(GVirNetworkDHCPLease *lease) lease->priv = GVIR_NETWORK_DHCP_LEASE_GET_PRIVATE(lease); }
+#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES GVirNetworkDHCPLease *gvir_network_dhcp_lease_new(virNetworkDHCPLeasePtr handle) { return g_object_new(GVIR_TYPE_NETWORK_DHCP_LEASE, "handle", handle, NULL); } +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */
/** * gvir_network_dhcp_lease_get_iface: @@ -144,7 +158,11 @@ const gchar *gvir_network_dhcp_lease_get_iface(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), NULL);
+#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->iface; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ }
/** @@ -157,7 +175,11 @@ gint64 gvir_network_dhcp_lease_get_expiry_time(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), -1);
+#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->expirytime; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return -1; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ }
/** @@ -170,7 +192,11 @@ gint gvir_network_dhcp_lease_get_ip_type(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), -1);
+#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->type; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return -1; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ }
/** @@ -183,7 +209,11 @@ const gchar *gvir_network_dhcp_lease_get_mac(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), NULL);
+#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->mac; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ }
/** @@ -196,7 +226,11 @@ const gchar *gvir_network_dhcp_lease_get_iaid(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), NULL);
+#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->iaid; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ }
/** @@ -209,7 +243,11 @@ const gchar *gvir_network_dhcp_lease_get_ip(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), NULL);
+#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->ipaddr; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ }
/** @@ -222,7 +260,11 @@ guint gvir_network_dhcp_lease_get_prefix(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), 0);
+#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->prefix; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return 0; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ }
/** @@ -235,7 +277,11 @@ const gchar *gvir_network_dhcp_lease_get_hostname(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), NULL);
+#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->hostname; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ }
/** @@ -248,5 +294,9 @@ const gchar *gvir_network_dhcp_lease_get_client_id(GVirNetworkDHCPLease *lease) { g_return_val_if_fail(GVIR_IS_NETWORK_DHCP_LEASE(lease), NULL);
+#ifdef HAVE_VIR_NETWORK_GET_DHCP_LEASES return lease->priv->handle->clientid; +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ } 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.
num_leases = virNetworkGetDHCPLeases(network->priv->handle, mac, &leases, flags); if (num_leases < 0) { gvir_set_error_literal(err, GVIR_NETWORK_ERROR, @@ -277,4 +282,11 @@ GList *gvir_network_get_dhcp_leases(GVirNetwork *network, free(leases);
return g_list_reverse(ret); +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + (void)mac; + gvir_set_error_literal(err, GVIR_NETWORK_ERROR, + 0, + "Unable to get network DHCP leases"); + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ } -- 2.4.3
-- Regards, Zeeshan Ali (Khattak) ________________________________________ Befriend GNOME: http://www.gnome.org/friends/
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@redhat.com> 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 ?
num_leases = virNetworkGetDHCPLeases(network->priv->handle, mac, &leases, flags); if (num_leases < 0) { gvir_set_error_literal(err, GVIR_NETWORK_ERROR, @@ -277,4 +282,11 @@ GList *gvir_network_get_dhcp_leases(GVirNetwork *network, free(leases);
return g_list_reverse(ret); +#else /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ + (void)mac; + gvir_set_error_literal(err, GVIR_NETWORK_ERROR, + 0, + "Unable to get network DHCP leases"); + return NULL; +#endif /* HAVE_VIR_NETWORK_GET_DHCP_LEASES */ }
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jul 21, 2015 at 4:09 PM, Daniel P. Berrange <berrange@redhat.com> 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@redhat.com> 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/
On Tue, Jul 21, 2015 at 03:20:02PM +0100, Daniel P. Berrange wrote:
Going back on what I said a few hours ago....
On closer inspection, I re-discovered that we have already taken the approach of conditionally compiling code in libvirt-gobject to avoid increasing the min libvirt. So given this, I would like to avoid increasing the min required libvirt in the release we're about to do today for benefit of GNOME boxes.
I think we can still have a valid discussion about increasing the min libvirt after this release is done, since I think that 0.10.2 is really far too old. It is based on RHEL-6 vintage libvirt and that's not really useful due to lack of gobject introspection.
Yes, especially with the dep on glib 2.36 (released in March 2013) which is fairly new compared to libvirt 0.10.2 (just checked, it's only 6 months older than glib 2.36 actually).
So to clarify, I think we should set a clear policy on what platforms we're going to target going forward. This will let us make an easy & clear decision about when a patch needs to use the conditional compilation approach.
Fine with me, I agree with the targets you mentioned in your other email (ie RHEL 7.0 and other distros from the same timeframe). We'll probably want to make that "the el7 release or equivalent which was current N yeras before/N releases before the latest", as by the time we reach RHEL 7.7, 7.0 will be much less relevant imo. Christophe
participants (3)
-
Christophe Fergeau -
Daniel P. Berrange -
Zeeshan Ali (Khattak)