[libvirt] [PATCHv5 1/4] net-dhcp-leases: Implement the public APIs

[Daniel P. Berrange]

On Thu, Dec 12, 2013 at 06:00:17PM +0530, Nehal J Wani wrote:
> On Thu, Dec 12, 2013 at 5:14 PM, Daniel P. Berrange <berrange at redhat.com> wrote:
> > On Tue, Nov 26, 2013 at 02:35:58AM +0530, Nehal J Wani wrote:
> >> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> >> index 5aad75c..20ea40a 100644
> >> --- a/include/libvirt/libvirt.h.in
> >> +++ b/include/libvirt/libvirt.h.in
> >> +
> >> +typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases;
> >> +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr;
> >> +struct _virNetworkDHCPLeases {
> >
> > s/Leases/Lease/ -  each struct only stores a single lease
> >
> >> +    char *interface;            /* Network interface name */
> >> +    long long expirytime;       /* Seconds since epoch */
> >> +    int type;                   /* virIPAddrType */
> >> +    char *mac;                  /* MAC address */
> >> +    char *iaid;                 /* IAID */
> >> +    char *ipaddr;               /* IP address */
> >> +    unsigned int prefix;        /* IP address prefix */
> >> +    char *hostname;             /* Hostname */
> >> +    char *clientid;             /* Client ID or DUID */
> >> +};
> >
> >> @@ -2019,6 +2022,7 @@ libvirt_setuid_rpc_client_la_SOURCES =          \
> >>               util/virtypedparam.c            \
> >>               util/viruri.c                   \
> >>               util/virutil.c                  \
> >> +             util/virmacaddr.c               \
> >>               util/viruuid.c                  \
> >>               conf/domain_event.c             \
> >>               rpc/virnetsocket.c              \
> >
> > Don't add this here.
> 
> The code didn't compile without making the above addition. It kept
> throwing the error:
> 
> ../src/.libs/libvirt-setuid-rpc-client.a(libvirt_setuid_rpc_client_la-libvirt.o):
> In function `virNetworkGetDHCPLeasesForMAC':
> /home/Wani/Hobby/libvirt/src/libvirt.c:22187: undefined reference to
> `virMacAddrParse'

That'd go away if you remove that check from the public API
and put it in the driver code instead.

> >> diff --git a/src/libvirt.c b/src/libvirt.c
> >> index eff44eb..9a0b872 100644
> >> --- a/src/libvirt.c
> >> +++ b/src/libvirt.c
> >> @@ -68,6 +68,7 @@
> >>  #include "virstring.h"
> >>  #include "virutil.h"
> >>  #include "virtypedparam.h"
> >> +#include "virmacaddr.h"
> >>
> >>  #ifdef WITH_TEST
> >>  # include "test/test_driver.h"
> >
> >> +int
> >> +virNetworkGetDHCPLeasesForMAC(virNetworkPtr network,
> >> +                              const char *mac,
> >> +                              virNetworkDHCPLeasesPtr **leases,
> >> +                              unsigned int flags)
> >> +{
> >> +    virConnectPtr conn;
> >> +    virMacAddr addr;
> >> +
> >> +    VIR_DEBUG("network=%p, mac=%s, leases=%p, flags=%x",
> >> +               network, mac, leases, flags);
> >> +
> >> +    virResetLastError();
> >> +
> >> +    if (leases)
> >> +        *leases = NULL;
> >> +
> >> +    virCheckNonNullArgGoto(mac, error);
> >> +
> >> +    if (!VIR_IS_CONNECTED_NETWORK(network)) {
> >> +        virLibNetworkError(VIR_ERR_INVALID_NETWORK, __FUNCTION__);
> >> +        virDispatchError(NULL);
> >> +        return -1;
> >> +    }
> >> +
> >> +    /* Validate the MAC address */
> >> +    if (virMacAddrParse(mac, &addr) < 0) {
> >> +        virReportInvalidArg(mac, "%s",
> >> +                            _("Given MAC Address doesn't comply "
> >> +                              "with the standard (IEEE 802) format"));
> >> +        goto error;
> >> +    }
> >
> > Don't do this here - it is the job of driver APIs todo semantic
> > validation of parameters.
> 
> Do you mean, I need to put this check inside
> networkGetDHCPLeasesForMAC in src/network/bridge_driver.c ?

If you need to parse it, then yes, but if you don't need to parse
it then don't.

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 :|