8:15 p.m.
On 09/26/2013 02:08 AM, Nehal J Wani wrote:
Implement RPC calls for virNetworkGetDHCPLeases,
virNetworkGetDHCPLeasesForMAC
---
daemon/remote.c | 157 ++++++++++++++++++++++++++++++++++++++++
src/remote/remote_driver.c | 169 ++++++++++++++++++++++++++++++++++++++++++-
src/remote/remote_protocol.x | 59 ++++++++++++++-
src/remote_protocol-structs | 47 ++++++++++++
src/rpc/gendispatch.pl | 1 +
5 files changed, 429 insertions(+), 4 deletions(-)
Reviewing in a different order (pro-tip: you can create a file that lists:
*.x
*.h
*
then run 'git diff -O/path/to/file' or 'git send-email -O/path/to/file'
and git will automatically arrange the diff in the order that makes more
logical sense to reviewers)
diff --git a/src/remote/remote_protocol.x
b/src/remote/remote_protocol.x
index 8d17bad..c630951 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -2849,6 +2854,46 @@ struct remote_connect_get_cpu_model_names_ret
{
int ret;
};
+union remote_interface_id switch (int type) {
+case VIR_IP_ADDR_TYPE_IPV4:
+ remote_nonnull_string mac;
+case VIR_IP_ADDR_TYPE_IPV6:
+ unsigned hyper iaid;
+};
+
+struct remote_network_dhcp_lease {
+ hyper expirytime;
+ remote_interface_id id;
+ remote_nonnull_string ipaddr;
+ remote_string hostname;
+ remote_string clientid;
+ int type;
Similar to 1/4, I'd list 'type' before 'id', rather than after.
Once remote_network_dhcp_lease id defined correctly, the rest of this
file looks okay.
diff --git a/daemon/remote.c b/daemon/remote.c
index 9497cc1..770f62a 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -88,6 +88,7 @@ static void make_nonnull_node_device(remote_nonnull_node_device
*dev_dst, virNod
static void make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr
secret_src);
static void make_nonnull_nwfilter(remote_nonnull_nwfilter *net_dst, virNWFilterPtr
nwfilter_src);
static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst,
virDomainSnapshotPtr snapshot_src);
+static void make_dhcp_lease(remote_network_dhcp_lease *lease_dst,
virNetworkDHCPLeasesPtr lease_src);
Can we hoist that function into topological order, instead of having to
use a forward declaration? Furthermore, this function really should
return an int, so that we can detect OOM failure (yes, the existing
make_nonnull_* functions are buggy in that they mishandle OOM, but
that's pre-existing and you don't have to worry about it).
+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)
+{
+
+ if (leases && nleases) {
+ if (VIR_ALLOC_N(ret->leases.leases_val, nleases) < 0)
+ goto cleanup;
+
+ ret->leases.leases_len = nleases;
+ for (i = 0; i < nleases; i++)
+ make_dhcp_lease(ret->leases.leases_val + i, leases[i]);
make_dhcp_lease can fail due to OOM; if it does, you ought to gracefully
recover.
+
+ mac = args->mac;
+
+ if ((nleases = virNetworkGetDHCPLeasesForMAC(net, mac,
You could just directly use args->mac here instead of going through the
effort of declaring a local variable 'mac' (probably leftovers from
trying to use remote_string instead of remote_nonnull_string in an
earlier patch version).
+ if (leases && nleases) {
+ if (VIR_ALLOC_N(ret->leases.leases_val, nleases) < 0)
+ goto cleanup;
+
+ ret->leases.leases_len = nleases;
+ for (i = 0; i < nleases; i++)
+ make_dhcp_lease(ret->leases.leases_val + i, leases[i]);
Again, this should gracefully handle OOM.
+
+/* Copy contents of virNetworkDHCPLeasesPtr to remote_network_dhcp_lease */
+static void
+make_dhcp_lease(remote_network_dhcp_lease *lease_dst, virNetworkDHCPLeasesPtr
lease_src)
+{
+ char **hostname_tmp = NULL;
+ char **clientid_tmp = NULL;
+ ignore_value(VIR_ALLOC_QUIET(hostname_tmp));
+ ignore_value(VIR_ALLOC_QUIET(clientid_tmp));
Rather than ignoring failure, we should clean up the intermediate object
and return -1 to the caller, so that they can then clean up all the
earlier array slots and propagate the error to the other end of RPC.
+
+ lease_dst->expirytime = lease_src->expirytime;
+ lease_dst->type = lease_src->type;
+ lease_dst->prefix = lease_src->prefix;
+ if (lease_src->type == VIR_IP_ADDR_TYPE_IPV4)
+ ignore_value(VIR_STRDUP_QUIET(lease_dst->id.remote_interface_id_u.mac,
+ lease_src->id.mac));
+ else
+ lease_dst->id.remote_interface_id_u.iaid = lease_src->id.iaid;
+ ignore_value(VIR_STRDUP_QUIET(lease_dst->ipaddr, lease_src->ipaddr));
+ ignore_value(VIR_STRDUP_QUIET(*hostname_tmp, lease_src->hostname));
+ ignore_value(VIR_STRDUP_QUIET(*clientid_tmp, lease_src->clientid));
Why two spaces after comma?
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 96ccb99..95910b6 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -14,7 +14,7 @@
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
- * You should have received a copy of the GNU Lesser General Public
+ * You should have received a make of the GNU Lesser General Public
Whoops. Bogus commit hunk.
@@ -150,6 +150,7 @@ static void
make_nonnull_storage_vol(remote_nonnull_storage_vol *vol_dst, virSto
static void make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr
secret_src);
static void make_nonnull_nwfilter(remote_nonnull_nwfilter *nwfilter_dst, virNWFilterPtr
nwfilter_src);
static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst,
virDomainSnapshotPtr snapshot_src);
+static void make_dhcp_lease(virNetworkDHCPLeasesPtr lease_src, remote_network_dhcp_lease
*lease_dst);
Again, we shouldn't be copying bad practice. Make this function return
int, and consider rearranging the file so that the helper function is
implemented first without needing a forward declaration (see
remoteSerializeTypedParameters as an example of a function that did a
bit better job of being declared in order and returning OOM failure).
static void remoteDomainEventQueue(struct private_data *priv,
virDomainEventPtr event);
/*----------------------------------------------------------------------*/
@@ -1058,7 +1059,7 @@ doRemoteClose(virConnectPtr conn, struct private_data *priv)
virObjectUnref(priv->qemuProgram);
priv->remoteProgram = priv->qemuProgram = priv->lxcProgram = NULL;
- /* Free hostname copy */
+ /* Free hostname make */
Another bogus hunk. Did you do some sort of global search-and-replace
s/copy/make/?
VIR_FREE(priv->hostname);
/* See comment for remoteType. */
@@ -3756,7 +3757,7 @@ static sasl_callback_t *remoteAuthMakeCallbacks(int *credtype, int
ncredtype)
*
* Builds up an array of libvirt credential structs, populating
* with data from the SASL interaction struct. These two structs
- * are basically a 1-to-1 copy of each other.
+ * are basically a 1-to-1 make of each other.
Bogus.
+static int
+remoteNetworkGetDHCPLeases(virNetworkPtr net,
+ virNetworkDHCPLeasesPtr **leases,
+ unsigned int flags)
+{
+ int rv = -1;
+ if (leases) {
+ 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;
+
+ make_dhcp_lease(leases_ret[i], &ret.leases.leases_val[i]);
Need to check for OOM failure in make_dhcp_lease.
@@ -7021,6 +7182,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 */
This will need to be bumped to 1.1.4.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org