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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: OpenPGP digital signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]