Re: [libvirt] [PATCHv2 2/4] net-dhcp-leases: Implement the remote protocol

On 13/09/13 05:53, Nehal J Wani wrote:

Implement RPC calls for virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC

daemon/remote.c * Define remoteSerializeNetworkDHCPLeases, remoteDispatchNetworkGetDHCPLeases * Define remoteDispatchNetworkGetDHCPLeasesForMAC

src/remote/remote_driver.c * Define remoteNetworkGetDHCPLeases * Define remoteNetworkGetDHCPLeasesForMAC

src/remote/remote_protocol.x * New RPC procedure: REMOTE_PROC_NETWORK_GET_DHCP_LEASES * Define structs remote_network_dhcp_leases, remote_network_get_dhcp_leases_args, remote_network_get_dhcp_leases_ret * New RPC procedure: REMOTE_PROC_NETWORK_GET_DHCP_LEASES_FOR_MAC * Define structs remote_network_dhcp_leases_for_mac, remote_network_get_dhcp_leases_for_mac_args, remote_network_get_dhcp_leases_for_mac_ret

src/remote_protocol-structs * New structs added

src/rpc/gendispatch.pl * Add exception (s/Dhcp/DHCP) for auto-generating names of the remote functions in ./daemon/remote_dispatch.h

--- daemon/remote.c | 133 ++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 150 +++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 50 ++++++++++++++- src/remote_protocol-structs | 32 +++++++++ src/rpc/gendispatch.pl | 1 + 5 files changed, 365 insertions(+), 1 deletion(-)

diff --git a/daemon/remote.c b/daemon/remote.c index 2aff7c1..de03739 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5137,7 +5137,140 @@ cleanup: return rv; }

+static int +remoteSerializeNetworkDHCPLeases(virNetworkDHCPLeasesPtr *leases, + int nleases, + remote_network_get_dhcp_leases_ret *ret) +{ + size_t i; + + if (nleases > REMOTE_NETWORK_DHCPLEASES_MAX) {

I think REMOTE_NETWORK_DHCP_LEASES_MAX is better.

+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of leases is %d, which exceeds max limit: %d"), + nleases, REMOTE_NETWORK_DHCPLEASES_MAX); + return -1; + } + + if (VIR_ALLOC_N(ret->leases.leases_val, nleases) < 0) + goto error;

If you "return -1;" here....

+ + ret->leases.leases_len = nleases; + + for (i = 0; i < nleases; i++) { + virNetworkDHCPLeasesPtr lease = leases[i]; + remote_network_dhcp_lease *lease_ret = &(ret->leases.leases_val[i]); + lease_ret->expirytime = lease->expirytime; + lease_ret->type = lease->type; + lease_ret->prefix = lease->prefix; + + if ((VIR_STRDUP(lease_ret->mac, lease->mac) < 0) || + (VIR_STRDUP(lease_ret->ipaddr, lease->ipaddr) < 0) || + (VIR_STRDUP(lease_ret->hostname, lease->hostname) < 0) || + (VIR_STRDUP(lease_ret->clientid, lease->clientid) < 0)) + goto error; + } + + return 0; + +error: + if (ret->leases.leases_val) {

then no need for this checking.

+ for (i = 0; i < nleases; i++) { + remote_network_dhcp_lease *lease_ret = &(ret->leases.leases_val[i]); + virNetworkDHCPLeaseFree((virNetworkDHCPLeasesPtr)lease_ret); + } + } + return -1; +} + +static int +remoteDispatchNetworkGetDHCPLeases(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_network_get_dhcp_leases_args *args, + remote_network_get_dhcp_leases_ret *ret) +{ + int rv = -1; + struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); + virNetworkDHCPLeasesPtr *leases = NULL; + virNetworkPtr net = NULL; + int nleases = 0; + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(net = get_nonnull_network(priv->conn, args->net))) + goto cleanup; + + if ((nleases = virNetworkGetDHCPLeases(net, &leases, args->flags)) < 0) + goto cleanup; + + if (remoteSerializeNetworkDHCPLeases(leases, nleases, ret) < 0) + goto cleanup;

+ rv = nleases; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (leases) { + size_t i; + for (i = 0; i < nleases; i++) { + virNetworkDHCPLeaseFree(leases[i]); + } + }

Network object "net" is leaked. [1]

+ VIR_FREE(leases); + return rv; +} + +static int +remoteDispatchNetworkGetDHCPLeasesForMAC(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_network_get_dhcp_leases_for_mac_args *args, + remote_network_get_dhcp_leases_for_mac_ret *ret) +{ + int rv = -1; + struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); + virNetworkDHCPLeasesPtr *leases = NULL; + virNetworkPtr net = NULL; + int nleases = 0; + const char *mac = NULL; + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(net = get_nonnull_network(priv->conn, args->net))) + goto cleanup; + + mac = args->mac ? *args->mac : NULL; + + if ((nleases = virNetworkGetDHCPLeasesForMAC(net, mac, &leases, args->flags)) < 0) + goto cleanup; + + if (remoteSerializeNetworkDHCPLeases(leases, nleases, + (remote_network_get_dhcp_leases_ret *)ret) < 0) + goto cleanup; + + rv = nleases; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (leases) { + size_t i; + for (i = 0; i < nleases; i++) { + virNetworkDHCPLeaseFree(leases[i]); + } + } + VIR_FREE(leases);

Same as [1]

+ return rv; +}

/*----- Helpers. -----*/

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 62e77a5..693de42 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6599,6 +6599,154 @@ done: return rv; }

+static int +remoteNetworkGetDHCPLeasesForMAC(virNetworkPtr net, + const char *mac, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags) +{ + int rv = -1; + size_t i; + struct private_data *priv = net->conn->networkPrivateData; + remote_network_get_dhcp_leases_for_mac_args args; + remote_network_get_dhcp_leases_for_mac_ret ret; + + virNetworkDHCPLeasesPtr *leases_ret; + remoteDriverLock(priv); + + make_nonnull_network(&args.net, net); + args.mac = mac ? (char **)&mac : NULL; + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + + if (call(net->conn, priv, 0, REMOTE_PROC_NETWORK_GET_DHCP_LEASES_FOR_MAC, + (xdrproc_t)xdr_remote_network_get_dhcp_leases_for_mac_args, (char *)&args, + (xdrproc_t)xdr_remote_network_get_dhcp_leases_for_mac_ret, (char *)&ret) == -1) { + goto done; + } + + if (ret.leases.leases_len > REMOTE_NETWORK_DHCPLEASES_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of leases is %d, which exceeds max limit: %d"), + ret.leases.leases_len, REMOTE_NETWORK_DHCPLEASES_MAX);

Likewise, s/DHCPLEASES/DHCP_LEASES/,

+ return -1; + goto cleanup; + } + + if (ret.leases.leases_len && + VIR_ALLOC_N(leases_ret, ret.leases.leases_len) < 0) {

[2] The API declares the returned array is NULL terminated, but here it doesn't, you should allocate one more element for it: VIR_ALLOC_N(leases_ret, ret.leases.leases_len + 1)

+ goto cleanup; + } + + for (i = 0; i < ret.leases.leases_len; i++) { + if (VIR_ALLOC(leases_ret[i]) < 0) + goto cleanup; + + virNetworkDHCPLeasesPtr lease = leases_ret[i]; + remote_network_dhcp_lease *lease_ret = &(ret.leases.leases_val[i]); + lease->expirytime = lease_ret->expirytime; + lease->type = lease_ret->type; + lease->prefix = lease_ret->prefix; + + if ((VIR_STRDUP(lease->mac, lease_ret->mac) < 0) || + (VIR_STRDUP(lease->ipaddr, lease_ret->ipaddr) < 0) || + (VIR_STRDUP(lease->hostname, lease_ret->hostname) < 0) || + (VIR_STRDUP(lease->clientid, lease_ret->clientid) < 0)) + goto cleanup; + } + + *leases = leases_ret; + leases_ret = NULL; + rv = ret.leases.leases_len; + +cleanup: + if (leases_ret) { + for (i = 0; i < ret.leases.leases_len; i++) + virNetworkDHCPLeaseFree(leases_ret[i]); + } + VIR_FREE(leases_ret); + xdr_free((xdrproc_t)xdr_remote_network_get_dhcp_leases_for_mac_ret, + (char *) &ret); + +done: + remoteDriverUnlock(priv); + return rv; +} + +static int +remoteNetworkGetDHCPLeases(virNetworkPtr net, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags) +{ + int rv = -1; + size_t i; + struct private_data *priv = net->conn->networkPrivateData; + remote_network_get_dhcp_leases_args args; + remote_network_get_dhcp_leases_ret ret; + + virNetworkDHCPLeasesPtr *leases_ret; + remoteDriverLock(priv); + + make_nonnull_network(&args.net, net); + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + + if (call(net->conn, priv, 0, REMOTE_PROC_NETWORK_GET_DHCP_LEASES, + (xdrproc_t)xdr_remote_network_get_dhcp_leases_args, (char *)&args, + (xdrproc_t)xdr_remote_network_get_dhcp_leases_ret, (char *)&ret) == -1) { + goto done; + } + + if (ret.leases.leases_len > REMOTE_NETWORK_DHCPLEASES_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of leases is %d, which exceeds max limit: %d"), + ret.leases.leases_len, REMOTE_NETWORK_DHCPLEASES_MAX);

s/DHCPLEASES/DHCP_LEASES/,

+ return -1; + goto cleanup; + } + + if (ret.leases.leases_len && + VIR_ALLOC_N(leases_ret, ret.leases.leases_len) < 0) {

Same problem as [2]

+ goto cleanup; + } + + for (i = 0; i < ret.leases.leases_len; i++) { + if (VIR_ALLOC(leases_ret[i]) < 0) + goto cleanup; + + virNetworkDHCPLeasesPtr lease = leases_ret[i]; + remote_network_dhcp_lease *lease_ret = &(ret.leases.leases_val[i]); + lease->expirytime = lease_ret->expirytime; + lease->type = lease_ret->type; + lease->prefix = lease_ret->prefix; + + if ((VIR_STRDUP(lease->mac, lease_ret->mac) < 0) || + (VIR_STRDUP(lease->ipaddr, lease_ret->ipaddr) < 0) || + (VIR_STRDUP(lease->hostname, lease_ret->hostname) < 0) || + (VIR_STRDUP(lease->clientid, lease_ret->clientid) < 0)) + goto cleanup; + } + + *leases = leases_ret; + leases_ret = NULL; + rv = ret.leases.leases_len; + +cleanup: + if (leases_ret) { + for (i = 0; i < ret.leases.leases_len; i++) + virNetworkDHCPLeaseFree(leases_ret[i]); + } + VIR_FREE(leases_ret); + xdr_free((xdrproc_t)xdr_remote_network_get_dhcp_leases_ret, + (char *) &ret); + +done: + remoteDriverUnlock(priv); + return rv; +} + static void remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event) { @@ -6958,6 +7106,8 @@ static virNetworkDriver network_driver = { .networkSetAutostart = remoteNetworkSetAutostart, /* 0.3.0 */ .networkIsActive = remoteNetworkIsActive, /* 0.7.3 */ .networkIsPersistent = remoteNetworkIsPersistent, /* 0.7.3 */ + .networkGetDHCPLeases = remoteNetworkGetDHCPLeases, /* 1.1.3 */ + .networkGetDHCPLeasesForMAC = remoteNetworkGetDHCPLeasesForMAC, /* 1.1.3 */ };

static virInterfaceDriver interface_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 85ad9ba..c455038 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -232,6 +232,11 @@ const REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX = 64; /* Upper limit on number of job stats */ const REMOTE_DOMAIN_JOB_STATS_MAX = 16;

+/* + * Upper limit on the maximum number of leases in one lease file + */ +const REMOTE_NETWORK_DHCPLEASES_MAX = 32768; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN];

@@ -2835,6 +2840,35 @@ struct remote_domain_event_device_removed_msg { remote_nonnull_string devAlias; };

+struct remote_network_dhcp_lease { + hyper expirytime; + remote_nonnull_string mac; + remote_nonnull_string ipaddr; + remote_nonnull_string hostname; + remote_nonnull_string clientid; + int type; + unsigned int prefix; +}; + +struct remote_network_get_dhcp_leases_args { + remote_nonnull_network net; + unsigned int flags; +}; + +struct remote_network_get_dhcp_leases_ret { + remote_network_dhcp_lease leases<REMOTE_NETWORK_DHCPLEASES_MAX>;

Likewise.

+}; + +struct remote_network_get_dhcp_leases_for_mac_args { + remote_nonnull_network net; + remote_string mac; + unsigned int flags; +}; + +struct remote_network_get_dhcp_leases_for_mac_ret { + remote_network_dhcp_lease leases<REMOTE_NETWORK_DHCPLEASES_MAX>;

Likewise. Osier