Re: [libvirt] [RFC] net-dhcp-leases: virNetworkDHCPLeases struct design

On Fri, Oct 11, 2013 at 12:36 AM, Eric Blake <eblake(a)redhat.com&gt; wrote: > On 10/08/2013 06:39 AM, Nehal J Wani wrote: >> Continued from https://www.redhat.com/archives/libvir-list/2013-October/msg00325.html >> > >> Since no one likes the idea of having a union, but we do want to >> support dhcpv6, following is the new design: >> >> struct _virNetworkDHCPLeases { >> char *interface; /* Network interface name (non-null) */ >> long long expirytime; /* Seconds since epoch (non-null) */ > > (non-null) makes no sense for an integer. On the other hand, should we > document a sentinel for unknown (or infinite) lease expiration? > dnsmasq will return the value 0 for expirytime in case of infinite expiration. >> int type; /* virIPAddrType (non-null) */ > > (non-null) makes no sense for an integer. > >> unsigned int prefix; /* IP address prefix (non-null) */ > > (non-null) makes no sense for an integer; should we document that 0 > means unknown? > >> char *mac; /* MAC address (mostly non-null, >> except rare cases) */ > > I guess it could be NULL for the generic virNetworkDHCPLeases query; but > I don't see how it can possibly be NULL for the specific per-MAC > virNetworkDHCPLeases ForMAC(). Probably also worth documenting that it > is in ASCII form (not raw bytes). If you implement it as remote_string > on the RPC side, then you are guaranteeing that it is non-NULL; are we > hurting ourselves if we make that guarantee? Well, in the discussion with dnsmasq developers, Simon had said, "if you're interested in the MAC addresses of clients, the very latest dnsmasq code can determine that in most cases." Since it says 'most cases', and we don't want to take risks, I was being skeptical in keeping it NON-NULL. In the case for virNetworkDHCPLeasesForMAC(), if the user passes NULL for@mac, the API will automatically throw an error: + /* 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; + } So, rewriting: /** * _virNetworkDHCPLeases: * For DHCPv4, the information returned: * - Network Interface Name * - Expiry Time * - MAC address (can be NULL, only in rare cases) * - IAID (NULL) * - IPv4 address (with type and prefix) * - Hostname (can be NULL) * - Client ID (can be NULL) * * For DHCPv6, the information returned: * - Network Interface Name * - Expiry Time * - MAC address (can be NULL, only in rare cases) * - IAID (can be NULL, only in rare cases) * - IPv6 address (with type and prefix) * - Hostname (can be NULL) * - Client DUID * * Note: @mac, @iaid, @ipaddr, @clientid are in ASCII form, not raw bytes. * Note: @expirytime can 0, in case the lease is for infinite time. */ struct _virNetworkDHCPLeases { char *interface; /* Network interface name */ long long expirytime; /* Seconds since epoch */ int type; /* virIPAddrType */ unsigned int prefix; /* IP address prefix */ char *mac; /* MAC address */ char *iaid; /* IAID */ char *ipaddr; /* IP address */ char *hostname; /* Hostname */ char *clientid; /* Client ID or DUID */ }; /* Remote struct */ struct remote_network_dhcp_lease { remote_nonnull_string interface; hyper expirytime; int type; unsigned int prefix; remote_string mac; remote_string iaid; remote_nonnull_string ipaddr; remote_string hostname; remote_string clientid; };