[libvirt] [RFC] net-dhcp-leases: virNetworkDHCPLeases struct design
Continued from https://www.redhat.com/archives/libvir-list/2013-October/msg00325.html Earlier, the virNetworkDHCPLeases struct design was: struct _virNetworkDHCPLeases { long long expirytime; /* Seconds since epoch */ union { char *mac; /* MAC address */ unsigned long iaid; /* Identity association identifier (IAID) */ } id; char *ipaddr; /* IP address */ char *hostname; /* Hostname */ char *clientid; /* Client ID or DUID */ int type; /* virIPAddrType */ unsigned int prefix; /* IP address prefix */ }; 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) */ int type; /* virIPAddrType (non-null) */ unsigned int prefix; /* IP address prefix (non-null) */ char *mac; /* MAC address (mostly non-null, except rare cases) */ char *iaid; /* IAID (ipv6) (null, in case of ipv4) */ char *ipaddr; /* IP address (non-null) */ char *hostname; /* Hostname (can be null) */ char *clientid; /* Client ID(ipv4) or DUID(ipv6) (can be null, in case of ipv4) */ }; -- Nehal J Wani
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) */ int type; /* virIPAddrType (non-null) */ unsigned int prefix; /* IP address prefix (non-null) */ char *mac; /* MAC address (mostly non-null, except rare cases) */ char *iaid; /* IAID (ipv6) (null, in case of ipv4) */ char *ipaddr; /* IP address (non-null) */ char *hostname; /* Hostname (can be null) */ char *clientid; /* Client ID(ipv4) or DUID(ipv6) (can be null, in case of ipv4) */ }; -- Nehal J Wani
ping. I would like to continue on v5 of the lease API series, if the above struct gets acknowledged. -- Nehal J Wani
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?
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?
char *iaid; /* IAID (ipv6) (null, in case of ipv4) */ char *ipaddr; /* IP address (non-null) */
Maybe mention that it is in ASCII form, not raw bytes (maybe mention that once, at the top of the struct, since it now applies to both MAC and IP strings).
char *hostname; /* Hostname (can be null) */ char *clientid; /* Client ID(ipv4) or DUID(ipv6) (can be null, in case of ipv4) */ };
Seems like a reasonable struct to me. I'm not an IPv6 expert, if anyone else wants to chime in, but it's certainly looking better for use in a v5 series than what we were debating in v4. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Oct 11, 2013 at 12:36 AM, Eric Blake <eblake@redhat.com> 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; }; -- Nehal J Wani
On Fri, Oct 11, 2013 at 1:49 AM, Nehal J Wani <nehaljw.kkd1@gmail.com> wrote:
On Fri, Oct 11, 2013 at 12:36 AM, Eric Blake <eblake@redhat.com> 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; };
danpb, laine, would you like to share your views? -- Nehal J Wani
On Tue, Oct 08, 2013 at 06:09:22PM +0530, Nehal J Wani wrote:
Continued from https://www.redhat.com/archives/libvir-list/2013-October/msg00325.html
Earlier, the virNetworkDHCPLeases struct design was:
struct _virNetworkDHCPLeases { long long expirytime; /* Seconds since epoch */ union { char *mac; /* MAC address */ unsigned long iaid; /* Identity association identifier (IAID) */ } id; char *ipaddr; /* IP address */ char *hostname; /* Hostname */ char *clientid; /* Client ID or DUID */ int type; /* virIPAddrType */ unsigned int prefix; /* IP address prefix */ };
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) */ int type; /* virIPAddrType (non-null) */ unsigned int prefix; /* IP address prefix (non-null) */ char *mac; /* MAC address (mostly non-null, except rare cases) */ char *iaid; /* IAID (ipv6) (null, in case of ipv4) */ char *ipaddr; /* IP address (non-null) */
I'd suggest the 'prefix' should be placed here, since it is associated with the 'ipaddr' field.
char *hostname; /* Hostname (can be null) */ char *clientid; /* Client ID(ipv4) or DUID(ipv6) (can be null, in case of ipv4) */ };
ACK, not convinced we need 'clientid', but it doesn't harm us to expose it just in case. 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 :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Nehal J Wani