On Mon, Sep 16, 2013 at 11:31:27PM +0530, Nehal J Wani wrote:
> +static int
> +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"),
Please make your email client not mangle line breaks when you are
replying. It makes it really hard to follow quoted text.
> + 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;
> +
> + 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:
> + for (i = 0; i < nleases; i++) {
> + remote_network_dhcp_lease *lease_ret =
&(ret->leases.leases_val[i]);
> + virNetworkDHCPLeaseFree((virNetworkDHCPLeasesPtr)lease_ret);
[1]
> + }
> + 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]);
> + }
> + }
> + 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;
> +
> + if ((nleases = virNetworkGetDHCPLeasesForMAC(net, mac, &leases,
args->flags)) < 0)
> + goto cleanup;
> +
> + if (remoteSerializeNetworkDHCPLeases(leases, nleases,
> +
(remote_network_get_dhcp_leases_ret *)ret) < 0)
[2]
> + goto cleanup;
> +
> + rv = nleases;
> +
I am a little skeptical about the typecasts involved in [1] and [2]
Specifically for [2], although the APIs are different, the struct is same,
only the name is different. But, what would be the other options?
One option: (Suggested by Osier)
Change remoteSerializeNetworkDHCPLeases to
+remoteSerializeNetworkDHCPLeases(virNetworkDHCPLeasesPtr *leases,
+ int nleases,
+ (void *) ret,
+ int method)
where the datatype for variable in which ret will be copied, will be
decided based on the method (method's value will be taken from an enum
or something similar, e.g: 1->DHCPLeases, 2->DHCPLeasesForMAC)
OR
Second Option: Since the APIs should be independent, make another function:
remoteSerializeNetworkDHCPLeasesForMAC(...), but that will lead to code
redundancy.
The commonality between the methods is the 'remote_network_dhcp_lease'
struct. So instead of creating a remoteSerializeNetworkDHCPLeases
which serializes a list of leases. Make a helper method which just
serializes a single "remote_network_dhcp_lease" instance. Push the
list iteration code up into the parent methods.
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 :|