[libvirt] [PATCHv3 2/4] net-dhcp-leases: Implement the remote protocol
Eric Blake
eblake at redhat.com
Mon Sep 16 18:33:18 UTC 2013
On 09/15/2013 11:49 PM, Nehal J Wani wrote:
> Implement RPC calls for virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC
>
>
A lot of my comments below are similar to what I did on Guiseppe's
patch; you may want to closely study:
https://www.redhat.com/archives/libvir-list/2013-September/msg00808.html
> diff --git a/daemon/remote.c b/daemon/remote.c
> index 2aff7c1..5252893 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -5137,7 +5137,140 @@ cleanup:
> return rv;
> }
>
> +static int
Lately, we've been preferring a style of 2 blank lines between functions.
> +remoteSerializeNetworkDHCPLeases(virNetworkDHCPLeasesPtr *leases,
> + int nleases,
> + remote_network_get_dhcp_leases_ret *ret)
> +{
> + size_t i;
> +
> + if (nleases > REMOTE_NETWORK_DHCP_LEASES_MAX) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Number of leases is %d, which exceeds max limit: %d"),
> + nleases, REMOTE_NETWORK_DHCP_LEASES_MAX);
> + return -1;
> + }
> +
> + if (VIR_ALLOC_N(ret->leases.leases_val, nleases) < 0)
> + return -1;
> +
> + ret->leases.leases_len = nleases;
I wouldn't set this...
> +
> + 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;
> + }
...until here...
> +
> + return 0;
> +
> +error:
> + for (i = 0; i < nleases; i++) {
> + remote_network_dhcp_lease *lease_ret = &(ret->leases.leases_val[i]);
> + virNetworkDHCPLeaseFree((virNetworkDHCPLeasesPtr)lease_ret);
> + }
...so that the caller doesn't try to iterate over bogus
remote_network_dhcp_lease data on error.
> + 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)
This doesn't allow a user to efficiently request count-only semantics.
In comparison with our ListAll* apis, doing that would require the use
of an args->need_results member, where you can pass NULL instead of
&leases if the user doesn't care about the results.
> + goto cleanup;
> +
> + if (remoteSerializeNetworkDHCPLeases(leases, nleases, ret) < 0)
> + goto cleanup;
>
> + rv = nleases;
The odd spacing of this non-patched blank line in the middle is the
result of you not using 2 blanks between functions. rv must be 0, not
positive, on success; the RPC client can reconstruct nleases from
ret->leases.leases_val. Then again, if you plan on supporting NULL
leases for count-only operation, you need to add ret->ret as a way to
pass the count in the absence of a return array.
> +
> +cleanup:
> + if (rv < 0)
> + virNetMessageSaveError(rerr);
> + if (leases) {
> + size_t i;
> + for (i = 0; i < nleases; i++) {
> + virNetworkDHCPLeaseFree(leases[i]);
> + }
> + }
> + VIR_FREE(leases);
> + virNetworkFree(net);
> + 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;
args->mac should be non-NULL (since you already reject NULL at the API
level).
> +
> + 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;
rv must be 0 on success.
> +
> +cleanup:
> + if (rv < 0)
> + virNetMessageSaveError(rerr);
> + if (leases) {
> + size_t i;
> + for (i = 0; i < nleases; i++) {
> + virNetworkDHCPLeaseFree(leases[i]);
> + }
> + }
> + VIR_FREE(leases);
> + virNetworkFree(net);
> + return rv;
> +}
>
> /*----- Helpers. -----*/
>
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 62e77a5..92ac8ef 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)
> +{
Why is the ForMAC version first here, but last in all the other hunks
where you add the two functions?
> + 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;
Uninitialized...
> + remoteDriverLock(priv);
> +
> + make_nonnull_network(&args.net, net);
> + args.mac = mac ? (char **)&mac : NULL;
Again, I think args.mac is typed incorrectly. You should support
args.need_results to allow passing a NULL leases through to the driver.
> + 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_DHCP_LEASES_MAX) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Number of leases is %d, which exceeds max limit: %d"),
> + ret.leases.leases_len, REMOTE_NETWORK_DHCP_LEASES_MAX);
> + return -1;
> + goto cleanup;
Dead goto. The return -1 is wrong. With that removed,
...leases_ret is still uninitialized...
> + }
> +
> + if (ret.leases.leases_len &&
No need to populate leases_ret if the user passes leases==NULL.
> + VIR_ALLOC_N(leases_ret, ret.leases.leases_len + 1) < 0) {
> + 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;
I'm not sure if it's worth some transfer semantics instead of
VIR_STRDUP, as in:
lease->mac = lease_ret->mac;
lease_ret->mac = NULL;
On the one hand, it avoids some malloc pressure; on the other, it's a
little bit harder to follow all paths to make sure there are no leaks or
double frees.
> + }
> +
> + *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]);
...and you dereference an uninit variable to start freeing random data.
Hello SIGSEGV.
> + }
> + 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;
Same SEGSEGV on uninit data if something else fails.
> + 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_DHCP_LEASES_MAX) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Number of leases is %d, which exceeds max limit: %d"),
> + ret.leases.leases_len, REMOTE_NETWORK_DHCP_LEASES_MAX);
> + return -1;
> + goto cleanup;
Same dead goto.
> + }
> +
> + if (ret.leases.leases_len &&
> + VIR_ALLOC_N(leases_ret, ret.leases.leases_len + 1) < 0) {
> + 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;
> + }
Same issue about not handling user's NULL leases.
> +
> + *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..a29646b 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_DHCP_LEASES_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;
Needs a 'int need_results' member to pass NULL.
> +};
> +
> +struct remote_network_get_dhcp_leases_ret {
> + remote_network_dhcp_lease leases<REMOTE_NETWORK_DHCP_LEASES_MAX>;
Needs a 'int ret' member to handle NULL leases.
> +};
> +
> +struct remote_network_get_dhcp_leases_for_mac_args {
> + remote_nonnull_network net;
> + remote_string mac;
This should be remote_nonnull_string (since the API already rejects NULL).
> + unsigned int flags;
> +};
> +
> +struct remote_network_get_dhcp_leases_for_mac_ret {
> + remote_network_dhcp_lease leases<REMOTE_NETWORK_DHCP_LEASES_MAX>;
> +};
Same issues about handling NULL leases.
> +
> /*----- Protocol. -----*/
>
> /* Define the program number, protocol version and procedure numbers here. */
> @@ -4998,5 +5032,19 @@ enum remote_procedure {
> * @generate: both
> * @acl: none
> */
> - REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 311
> + REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 311,
> +
> + /**
> + * @generate: none
> + * @priority: high
Is high priority appropriate? Is this something where we are always
guaranteed that we will never need to block or interact with external
processes to get the information, even if we change our implementation
away from dnsmasq to something else?
> + * @acl: network:read
> + */
> + REMOTE_PROC_NETWORK_GET_DHCP_LEASES = 312,
> +
> + /**
> + * @generate: none
> + * @priority: high
> + * @acl: network:read
> + */
> + REMOTE_PROC_NETWORK_GET_DHCP_LEASES_FOR_MAC = 313
> };
> diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
> index 4e27aae..9b625e8 100644
> --- a/src/remote_protocol-structs
> +++ b/src/remote_protocol-structs
Be sure to regenerate this after fixing the .x file. You may also have
some (trivial) conflicts, depending on whether other new APIs go in first.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130916/83105a97/attachment-0001.sig>
More information about the libvir-list
mailing list