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