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

Re: [libvirt] [PATCHv4 2/5] domifaddr: Implement the remote protocol



On Sun, Aug 25, 2013 at 04:45:42AM +0530, Nehal J Wani wrote:
> diff --git a/daemon/remote.c b/daemon/remote.c
> index 03d5557..44d7ff2 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -5025,7 +5025,134 @@ cleanup:
>      return rv;
>  }
>  
> +static int
> +remoteSerializeDomainInterfacePtr(virDomainInterfacePtr *ifaces,
> +                                  unsigned int ifaces_count,
> +                                  remote_domain_interfaces_addresses_ret *ret)
> +{
> +    size_t i, j;
> +
> +    if (ifaces_count > REMOTE_DOMAIN_INTERFACE_MAX) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Number of interfaces exceeds max limit!"));

When reporting this error, include the actual 'ifaces_count' value
and the limit in the error message. No need for an '!' either.

> +    if (VIR_ALLOC_N(ret->ifaces.ifaces_val, ifaces_count) < 0)
> +        return -1;
> +
> +    ret->ifaces.ifaces_len = ifaces_count;
> +
> +    for (i = 0; i < ifaces_count; i++) {
> +        virDomainInterfacePtr iface = ifaces[i];
> +        remote_domain_interface *iface_ret = &(ret->ifaces.ifaces_val[i]);
>  
> +        if ((VIR_STRDUP(iface_ret->name, iface->name)) < 0)
> +            goto cleanup;
> +
> +        if (iface->hwaddr) {
> +            char **hwaddr_p = NULL;
> +            if (VIR_ALLOC(hwaddr_p) < 0)
> +                goto cleanup;
> +            if (VIR_STRDUP(*hwaddr_p, iface->hwaddr) < 0) {
> +                VIR_FREE(hwaddr_p);
> +                goto cleanup;
> +            }
> +
> +            iface_ret->hwaddr = hwaddr_p;
> +        }
> +
> +        if (iface->naddrs > REMOTE_DOMAIN_IP_ADDR_MAX) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("Number of IP addresses exceeds max limit!"));

Again, include the actual values checked, and remove the '!'.

> +            goto cleanup;
> +        }
> +
> +        if (VIR_ALLOC_N(iface_ret->addrs.addrs_val,
> +                        iface->naddrs) < 0)
> +            goto cleanup;
> +
> +        iface_ret->addrs.addrs_len = iface->naddrs;
> +
> +        for (j = 0; j < iface->naddrs; j++) {
> +            virDomainIPAddressPtr ip_addr = &(iface->addrs[j]);
> +            remote_domain_ip_addr *ip_addr_ret =
> +                &(iface_ret->addrs.addrs_val[j]);
> +
> +            if (VIR_STRDUP(ip_addr_ret->addr, ip_addr->addr) < 0)
> +                goto cleanup;
> +
> +            ip_addr_ret->prefix = ip_addr->prefix;
> +            ip_addr_ret->type = ip_addr->type;
> +        }
> +    }
> +
> +    return 0;
> +
> +cleanup:
> +    if (ret->ifaces.ifaces_val) {
> +        for (i = 0; i < ifaces_count; i++) {
> +            remote_domain_interface *iface_ret = &(ret->ifaces.ifaces_val[i]);
> +            VIR_FREE(iface_ret->name);
> +            VIR_FREE(iface_ret->hwaddr);
> +            for (j = 0; j < iface_ret->addrs.addrs_len; j++) {
> +                remote_domain_ip_addr *ip_addr =
> +                    &(iface_ret->addrs.addrs_val[j]);
> +                VIR_FREE(ip_addr->addr);
> +            }
> +            VIR_FREE(iface_ret);
> +        }
> +        VIR_FREE(ret->ifaces.ifaces_val);
> +    }
> +
> +    return -1;
> +}
> +
> +static int
> +remoteDispatchDomainInterfacesAddresses(
> +    virNetServerPtr server ATTRIBUTE_UNUSED,
> +    virNetServerClientPtr client,
> +    virNetMessagePtr msg ATTRIBUTE_UNUSED,
> +    virNetMessageErrorPtr rerr,
> +    remote_domain_interfaces_addresses_args *args,
> +    remote_domain_interfaces_addresses_ret *ret)
> +{
> +    int rv = -1;
> +    virDomainPtr dom = NULL;
> +    virDomainInterfacePtr *ifaces = NULL;
> +    int ifaces_count = 0;
> +    struct daemonClientPrivate *priv =
> +        virNetServerClientGetPrivateData(client);
> +
> +    if (!priv->conn) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
> +        goto cleanup;
> +    }
> +
> +    if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
> +        goto cleanup;
> +
> +    if ((ifaces_count = virDomainInterfacesAddresses(dom, &ifaces, args->flags)) < 0)
> +        goto cleanup;
> +
> +    if (remoteSerializeDomainInterfacePtr(ifaces, ifaces_count, ret) < 0)
> +        goto cleanup;
> +
> +    rv = 0;
> +
> +cleanup:
> +    if (rv < 0)
> +        virNetMessageSaveError(rerr);
> +    if (dom)
> +        virDomainFree(dom);
> +    if (ifaces) {
> +        size_t i;
> +        for (i = 0; i < ifaces_count; i++)
> +            virDomainInterfaceFree(ifaces[i]);
> +        VIR_FREE(ifaces);
> +    }
> +    return rv;
> +}
>  
>  /*----- Helpers. -----*/
>  
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 71d0034..94eb63d 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -6477,6 +6477,90 @@ done:
>      return rv;
>  }
>  
> +static int
> +remoteDomainInterfacesAddresses(virDomainPtr dom,
> +                                virDomainInterfacePtr **ifaces,
> +                                unsigned int flags)
> +{
> +    int rv = -1;
> +    size_t i, j;
> +
> +    virDomainInterfacePtr *ifaces_ret = NULL;
> +    remote_domain_interfaces_addresses_args args;
> +    remote_domain_interfaces_addresses_ret ret;
> +
> +    struct private_data *priv = dom->conn->privateData;
> +
> +    args.flags = flags;
> +    make_nonnull_domain(&args.dom, dom);
> +
> +    remoteDriverLock(priv);
> +
> +    memset(&ret, 0, sizeof(ret));
> +
> +    if (call(dom->conn, priv, 0, REMOTE_PROC_DOMAIN_INTERFACES_ADDRESSES,
> +             (xdrproc_t)xdr_remote_domain_interfaces_addresses_args,
> +             (char *)&args,
> +             (xdrproc_t)xdr_remote_domain_interfaces_addresses_ret,
> +             (char *)&ret) == -1) {
> +        goto done;
> +    }
> +

You must check ret.ifaces.ifaces_len against REMOTE_DOMAIN_INTERFACE_MAX here
before allocating the array.

> +    if (ret.ifaces.ifaces_len &&
> +        VIR_ALLOC_N(ifaces_ret, ret.ifaces.ifaces_len) < 0)
> +        goto cleanup;
> +
> +    for (i = 0; i < ret.ifaces.ifaces_len; i++) {
> +        if (VIR_ALLOC(ifaces_ret[i]) < 0)
> +            goto cleanup;
> +
> +        virDomainInterfacePtr iface = ifaces_ret[i];
> +        remote_domain_interface *iface_ret = &(ret.ifaces.ifaces_val[i]);
> +
> +        if (VIR_STRDUP(iface->name, iface_ret->name) < 0)
> +            goto cleanup;
> +
> +        if (iface_ret->hwaddr &&
> +            VIR_STRDUP(iface->hwaddr, *iface_ret->hwaddr) < 0)
> +            goto cleanup;
> +
> +        iface->naddrs = iface_ret->addrs.addrs_len;

And here you must validate addrs_len against its limit too.

> +
> +        if (iface->naddrs) {
> +            if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0)
> +                goto cleanup;
> +
> +            for (j = 0; j < iface->naddrs; j++) {
> +                virDomainIPAddressPtr ip_addr = &(iface->addrs[j]);
> +                remote_domain_ip_addr *ip_addr_ret =
> +                    &(iface_ret->addrs.addrs_val[j]);
> +
> +                if (VIR_STRDUP(ip_addr->addr, ip_addr_ret->addr) < 0) {
> +                    goto cleanup;
> +                }
> +                ip_addr->prefix = ip_addr_ret->prefix;
> +                ip_addr->type = ip_addr_ret->type;
> +            }
> +        }
> +    }
> +    *ifaces = ifaces_ret;
> +    ifaces_ret = NULL;
> +
> +    rv = ret.ifaces.ifaces_len;
> +
> +cleanup:
> +    if (ifaces_ret) {
> +        for (i = 0; i < ret.ifaces.ifaces_len; i++)
> +            virDomainInterfaceFree(ifaces_ret[i]);
> +        VIR_FREE(ifaces_ret);
> +    }
> +    xdr_free((xdrproc_t)xdr_remote_domain_interfaces_addresses_ret,
> +             (char *) &ret);
> +done:
> +    remoteDriverUnlock(priv);
> +    return rv;
> +}

> @@ -2837,6 +2848,27 @@ struct remote_domain_event_device_removed_msg {
>      remote_nonnull_string devAlias;
>  };
>  
> +struct remote_domain_ip_addr {
> +    int type;
> +    remote_nonnull_string addr;
> +    int prefix;


Lets make this  'unsigned int' to match the public API change i suggested.

> +struct remote_domain_interface {
> +    remote_nonnull_string name;
> +    remote_string hwaddr;
> +    remote_domain_ip_addr addrs<REMOTE_DOMAIN_IP_ADDR_MAX>;
> +};
> +
> +struct remote_domain_interfaces_addresses_args {
> +    remote_nonnull_domain dom;
> +    unsigned int flags;
> +};
> +
> +struct remote_domain_interfaces_addresses_ret {
> +    remote_domain_interface ifaces<REMOTE_DOMAIN_INTERFACE_MAX>;
> +};


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


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