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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list