[libvirt] [PATCH] net: merge virNetworkGetDHCPLeases and virNetworkGetDHCPLeasesForMAC

[Peter Krempa]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140627/8e6c352c/attachment-0001.sig>