Re: [Xen-devel] [PATCH] libgnttab: Add support for Linux dma-buf

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Jul 23, 2018 at 03:27:25PM +0300, Oleksandr Andrushchenko wrote:
[...]
> diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
> index 8347ddd3d9cf..9765146f7eb6 100644
> --- a/tools/libs/gnttab/linux.c
> +++ b/tools/libs/gnttab/linux.c
> @@ -1,5 +1,6 @@
>  /*
>   * Copyright (c) 2007-2008, D G Murray <Derek.Murray@xxxxxxxxxxxx>
> + * Copyright (c) 2018, Oleksandr Andrushchenko, EPAM Systems Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -309,6 +310,118 @@ int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
>      return rc;
>  }
>  
> +int osdep_gnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
> +                                      uint32_t flags, uint32_t count,
> +                                      const uint32_t *refs,
> +                                      uint32_t *dmabuf_fd)
> +{
> +    struct ioctl_gntdev_dmabuf_exp_from_refs *from_refs;

You can set from_refs to NULL here, then ...

> +    int rc = 0;
> +
> +    if ( !count )
> +    {
> +        errno = EINVAL;
> +        return -1;

    rc = -1;
    goto out;

in all exit paths.

I don't like using two error handling styles. So please either change to
use goto only or delete the only goto in this function.

> +    }
> +
> +    from_refs = malloc(sizeof(*from_refs) +
> +                       (count - 1) * sizeof(from_refs->refs[0]));
> +    if ( !from_refs )
> +    {
> +        errno = ENOMEM;
> +        return -1;
> +    }
> +
> +    from_refs->flags = flags;
> +    from_refs->count = count;
> +    from_refs->domid = domid;
> +
> +    memcpy(from_refs->refs, refs, count * sizeof(from_refs->refs[0]));
> +
> +    if ( (rc = ioctl(xgt->fd, IOCTL_GNTDEV_DMABUF_EXP_FROM_REFS, from_refs)) 
> )
> +    {
> +        GTERROR(xgt->logger, "ioctl DMABUF_EXP_FROM_REFS failed");
> +        goto out;
> +    }
> +
> +    *dmabuf_fd = from_refs->fd;
> +
> +out:
> +    free(from_refs);
> +    return rc;
> +}
> +
> +int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
> +                                          uint32_t fd, uint32_t wait_to_ms)
> +{
> +    struct ioctl_gntdev_dmabuf_exp_wait_released wait;
> +    int rc;
> +
> +    wait.fd = fd;
> +    wait.wait_to_ms = wait_to_ms;
> +
> +    if ( (rc = ioctl(xgt->fd, IOCTL_GNTDEV_DMABUF_EXP_WAIT_RELEASED, &wait)) 
> ) {

{ should be on a new line.

> +        if ( errno == ENOENT ) {

Ditto.

> +            /* The buffer may have already been released. */
> +            errno = 0;
> +            rc = 0;
> +        } else
> +            GTERROR(xgt->logger, "ioctl DMABUF_EXP_WAIT_RELEASED failed");
> +    }
> +
> +    return rc;
> +}
> +
> +int osdep_gnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
> +                                    uint32_t fd, uint32_t count, uint32_t 
> *refs)
> +{
> +    struct ioctl_gntdev_dmabuf_imp_to_refs *to_refs;
> +    int rc = 0;
> +
> +    if ( !count )
> +    {
> +        errno = EINVAL;
> +        return -1;

Same comments on error handling apply to this function too.

The rest looks fine.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel