Re: [libvirt] [PATCH] net: merge virNetworkGetDHCPLeases and virNetworkGetDHCPLeasesForMAC
On Fri, Jun 27, 2014 at 09:45:09AM +0200, Peter Krempa wrote: > On 06/26/14 17:29, Ján Tomko wrote: > > On 06/26/2014 04:51 PM, Peter Krempa wrote: > >> Instead of maintaining two very similar APIs, add the "@mac" parameter > >> to virNetworkGetDHCPLeases and kill virNetworkGetDHCPLeasesForMAC. Both > >> of those functions would return data the same way, so making @mac an > >> optional filter simplifies a lot of stuff. [...] > > ACK with the debug message added and virsh functionality fixed. > > > > Jan > > > > I've resolved all your comments and pushed this patch. > > Thanks Not finished, this now break the libvirt-python build which expects virNetworkGetDHCPLeasesForMAC to be in the exported set of function, so a bit of cleanup is needed there, preferably before the release :-) Daniel -- Daniel Veillard | Open Source and Standards, Red Hat veill...@redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] net: merge virNetworkGetDHCPLeases and virNetworkGetDHCPLeasesForMAC
On 06/26/14 17:29, Ján Tomko wrote:
> On 06/26/2014 04:51 PM, Peter Krempa wrote:
>> Instead of maintaining two very similar APIs, add the "@mac" parameter
>> to virNetworkGetDHCPLeases and kill virNetworkGetDHCPLeasesForMAC. Both
>> of those functions would return data the same way, so making @mac an
>> optional filter simplifies a lot of stuff.
>> ---
>> daemon/remote.c | 69 +-
>> include/libvirt/libvirt.h.in | 6 +---
>> src/driver.h | 8 +
>> src/libvirt.c| 70
>> ++-
>> src/libvirt_public.syms | 1 -
>> src/network/bridge_driver.c | 69 +++---
>> src/remote/remote_driver.c | 71
>> ++--
>> src/remote/remote_protocol.x | 20 ++---
>> src/remote_protocol-structs | 15 +-
>> tools/virsh-network.c| 5 +---
>> 10 files changed, 35 insertions(+), 299 deletions(-)
>>
>
>> diff --git a/src/libvirt.c b/src/libvirt.c
>> index 566f984..49c9d16 100644
>> --- a/src/libvirt.c
>> +++ b/src/libvirt.c
>
>> @@ -21110,65 +21117,6 @@ virNetworkGetDHCPLeases(virNetworkPtr network,
>> return -1;
>> }
>>
>> -/**
>> - * virNetworkGetDHCPLeasesForMAC:
>> - * @network: Pointer to network object
>> - * @mac: ASCII formatted MAC address of an interface
>> - * @leases: Pointer to a variable to store the array containing details on
>> - * obtained leases, or NULL if the list is not required (just
>> returns
>> - * number of leases).
>> - * @flags: extra flags, not used yet, so callers should always pass 0
>> - *
>> - * The API fetches leases info of the interface which matches with the
>> - * given @mac. There can be multiple leases for a single @mac because this
>> - * API supports DHCPv6 too.
>> - *
>> - * Returns the number of leases found or -1 and sets @leases to NULL in
>> case of
>> - * error. On success, the array stored into @leases is guaranteed to have an
>> - * extra allocated element set to NULL but not included in the return count,
>> - * to make iteration easier. The caller is responsible for calling
>> - * virNetworkDHCPLeaseFree() on each array element, then calling free() on
>> @leases.
>> - *
>> - * See virNetworkGetDHCPLeases() for more details on list contents.
>> - */
>> -int
>> -virNetworkGetDHCPLeasesForMAC(virNetworkPtr network,
>> - const char *mac,
>> - virNetworkDHCPLeasePtr **leases,
>> - unsigned int flags)
>> -{
>> -virConnectPtr conn;
>> -
>> -VIR_DEBUG("network=%p, mac=%s, leases=%p, flags=%x",
>> - network, mac, leases, flags);
>
> You should add mac to the debug message at the start of the other API.
>
>> -
>> -virResetLastError();
>> -
>> -if (leases)
>> -*leases = NULL;
>> -
>> -virCheckNonNullArgGoto(mac, error);
>> -
>> -virCheckNetworkReturn(network, -1);
>
>
>> --- a/src/remote/remote_driver.c
>> +++ b/src/remote/remote_driver.c
>> @@ -7614,6 +7615,7 @@ remoteNetworkGetDHCPLeases(virNetworkPtr net,
>> remoteDriverLock(priv);
>>
>> make_nonnull_network(&args.net, net);
>> +args.mac = mac == NULL ? NULL : (char **) &mac;
>
> Nit: mac ? (char **) &mac : NULL would be IMO nicer.
>
>> args.flags = flags;
>> args.need_results = !!leases;
>>
>
>> diff --git a/tools/virsh-network.c b/tools/virsh-network.c
>> index 2d5b9be..e7499fa 100644
>> --- a/tools/virsh-network.c
>> +++ b/tools/virsh-network.c
>> @@ -1348,10 +1348,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd
>> *cmd)
>> if (!(network = vshCommandOptNetwork(ctl, cmd, &name)))
>> return false;
>>
>> -nleases = mac ? virNetworkGetDHCPLeasesForMAC(network, mac, &leases,
>> flags)
>> -: virNetworkGetDHCPLeases(network, &leases, flags);
>> -
>> -if (nleases < 0) {
>> +if ((nleases = virNetworkGetDHCPLeases(network, mac, &leases, flags) <
>> 0)) {
>
> Wrong parenthesising.
>
> ACK with the debug message added and virsh functionality fixed.
>
> Jan
>
I've resolved all your comments and pushed this patch.
Thanks
Peter
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] net: merge virNetworkGetDHCPLeases and virNetworkGetDHCPLeasesForMAC
On 06/26/2014 04:51 PM, Peter Krempa wrote:
> Instead of maintaining two very similar APIs, add the "@mac" parameter
> to virNetworkGetDHCPLeases and kill virNetworkGetDHCPLeasesForMAC. Both
> of those functions would return data the same way, so making @mac an
> optional filter simplifies a lot of stuff.
> ---
> daemon/remote.c | 69 +-
> include/libvirt/libvirt.h.in | 6 +---
> src/driver.h | 8 +
> src/libvirt.c| 70 ++-
> src/libvirt_public.syms | 1 -
> src/network/bridge_driver.c | 69 +++---
> src/remote/remote_driver.c | 71
> ++--
> src/remote/remote_protocol.x | 20 ++---
> src/remote_protocol-structs | 15 +-
> tools/virsh-network.c| 5 +---
> 10 files changed, 35 insertions(+), 299 deletions(-)
>
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 566f984..49c9d16 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -21110,65 +21117,6 @@ virNetworkGetDHCPLeases(virNetworkPtr network,
> return -1;
> }
>
> -/**
> - * virNetworkGetDHCPLeasesForMAC:
> - * @network: Pointer to network object
> - * @mac: ASCII formatted MAC address of an interface
> - * @leases: Pointer to a variable to store the array containing details on
> - * obtained leases, or NULL if the list is not required (just
> returns
> - * number of leases).
> - * @flags: extra flags, not used yet, so callers should always pass 0
> - *
> - * The API fetches leases info of the interface which matches with the
> - * given @mac. There can be multiple leases for a single @mac because this
> - * API supports DHCPv6 too.
> - *
> - * Returns the number of leases found or -1 and sets @leases to NULL in case
> of
> - * error. On success, the array stored into @leases is guaranteed to have an
> - * extra allocated element set to NULL but not included in the return count,
> - * to make iteration easier. The caller is responsible for calling
> - * virNetworkDHCPLeaseFree() on each array element, then calling free() on
> @leases.
> - *
> - * See virNetworkGetDHCPLeases() for more details on list contents.
> - */
> -int
> -virNetworkGetDHCPLeasesForMAC(virNetworkPtr network,
> - const char *mac,
> - virNetworkDHCPLeasePtr **leases,
> - unsigned int flags)
> -{
> -virConnectPtr conn;
> -
> -VIR_DEBUG("network=%p, mac=%s, leases=%p, flags=%x",
> - network, mac, leases, flags);
You should add mac to the debug message at the start of the other API.
> -
> -virResetLastError();
> -
> -if (leases)
> -*leases = NULL;
> -
> -virCheckNonNullArgGoto(mac, error);
> -
> -virCheckNetworkReturn(network, -1);
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -7614,6 +7615,7 @@ remoteNetworkGetDHCPLeases(virNetworkPtr net,
> remoteDriverLock(priv);
>
> make_nonnull_network(&args.net, net);
> +args.mac = mac == NULL ? NULL : (char **) &mac;
Nit: mac ? (char **) &mac : NULL would be IMO nicer.
> args.flags = flags;
> args.need_results = !!leases;
>
> diff --git a/tools/virsh-network.c b/tools/virsh-network.c
> index 2d5b9be..e7499fa 100644
> --- a/tools/virsh-network.c
> +++ b/tools/virsh-network.c
> @@ -1348,10 +1348,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd
> *cmd)
> if (!(network = vshCommandOptNetwork(ctl, cmd, &name)))
> return false;
>
> -nleases = mac ? virNetworkGetDHCPLeasesForMAC(network, mac, &leases,
> flags)
> -: virNetworkGetDHCPLeases(network, &leases, flags);
> -
> -if (nleases < 0) {
> +if ((nleases = virNetworkGetDHCPLeases(network, mac, &leases, flags) <
> 0)) {
Wrong parenthesising.
ACK with the debug message added and virsh functionality fixed.
Jan
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] net: merge virNetworkGetDHCPLeases and virNetworkGetDHCPLeasesForMAC
Instead of maintaining two very similar APIs, add the "@mac" parameter
to virNetworkGetDHCPLeases and kill virNetworkGetDHCPLeasesForMAC. Both
of those functions would return data the same way, so making @mac an
optional filter simplifies a lot of stuff.
---
daemon/remote.c | 69 +-
include/libvirt/libvirt.h.in | 6 +---
src/driver.h | 8 +
src/libvirt.c| 70 ++-
src/libvirt_public.syms | 1 -
src/network/bridge_driver.c | 69 +++---
src/remote/remote_driver.c | 71 ++--
src/remote/remote_protocol.x | 20 ++---
src/remote_protocol-structs | 15 +-
tools/virsh-network.c| 5 +---
10 files changed, 35 insertions(+), 299 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c
index 9ffc1cb..ea16789 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -6292,6 +6292,7 @@ remoteDispatchNetworkGetDHCPLeases(virNetServerPtr server
ATTRIBUTE_UNUSED,
goto cleanup;
if ((nleases = virNetworkGetDHCPLeases(net,
+ args->mac ? *args->mac : NULL,
args->need_results ? &leases : NULL,
args->flags)) < 0)
goto cleanup;
@@ -6336,74 +6337,6 @@ remoteDispatchNetworkGetDHCPLeases(virNetServerPtr
server ATTRIBUTE_UNUSED,
}
-static int
-remoteDispatchNetworkGetDHCPLeasesForMAC(virNetServerPtr server
ATTRIBUTE_UNUSED,
- virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr,
-
remote_network_get_dhcp_leases_for_mac_args *args,
-
remote_network_get_dhcp_leases_for_mac_ret *ret)
-{
-int rv = -1;
-size_t i;
-struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
-virNetworkDHCPLeasePtr *leases = NULL;
-virNetworkPtr net = NULL;
-int nleases = 0;
-
-if (!priv->conn) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
-goto cleanup;
-}
-
-if (!(net = get_nonnull_network(priv->conn, args->net)))
-goto cleanup;
-
-if ((nleases = virNetworkGetDHCPLeasesForMAC(net, args->mac,
- args->need_results ? &leases
: NULL,
- args->flags)) < 0)
-goto cleanup;
-
-if (nleases > REMOTE_NETWORK_DHCP_LEASES_MAX) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Number of leases is %d, which exceeds max limit:
%d"),
- nleases, REMOTE_NETWORK_DHCP_LEASES_MAX);
-return -1;
-}
-
-if (leases && nleases) {
-if (VIR_ALLOC_N(ret->leases.leases_val, nleases) < 0)
-goto cleanup;
-
-ret->leases.leases_len = nleases;
-
-for (i = 0; i < nleases; i++) {
-if (remoteSerializeDHCPLease(ret->leases.leases_val + i,
leases[i]) < 0)
-goto cleanup;
-}
-
-} else {
-ret->leases.leases_len = 0;
-ret->leases.leases_val = NULL;
-}
-
-ret->ret = nleases;
-
-rv = 0;
-
- cleanup:
-if (rv < 0)
-virNetMessageSaveError(rerr);
-if (leases) {
-for (i = 0; i < nleases; i++)
-virNetworkDHCPLeaseFree(leases[i]);
-VIR_FREE(leases);
-}
-virNetworkFree(net);
-return rv;
-}
-
-
/*- Helpers. -*/
/* get_nonnull_domain and get_nonnull_network turn an on-wire
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 594521e..032d6e6 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -5169,14 +5169,10 @@ struct _virNetworkDHCPLease {
void virNetworkDHCPLeaseFree(virNetworkDHCPLeasePtr lease);
int virNetworkGetDHCPLeases(virNetworkPtr network,
+const char *mac,
virNetworkDHCPLeasePtr **leases,
unsigned int flags);
-int virNetworkGetDHCPLeasesForMAC(virNetworkPtr network,
- const char *mac,
- virNetworkDHCPLeasePtr **leases,
- unsigned int flags);
-
/**
* virConnectNetworkEventGenericCallback:
* @conn: the connection pointer
diff --git a/src/driver.h b/src/driver.h
index 6e72e92..5018068 100644
--- a/src/driver.h
+++ b/src/driver.h
@@ -1184,15 +1184,10 @@ typedef int
typedef int
(*virDrvNetworkGetDHCPLeases)(virNetworkPtr network,
+ const char *mac,
virNetworkDHCPLeasePtr **leases,
u
