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

[Nehal J Wani <nehaljw.kkd1@xxxxxxxxx>]

> +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"),
> +                       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.

Eric, what would you do in these cases?

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list