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

[Oleksandr Andrushchenko <andr2000@gmail.com>]

On 08/20/2018 05:40 PM, Wei Liu wrote:
> 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@cl.cam.ac.uk>
>> + * 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.
Thank you,
I'll fix the above and send v1
> Wei.


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