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

[Daniel P. Berrange]

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 :|