<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Oct 17, 2017 at 10:46 AM, Chad Versace <span dir="ltr"><<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon 16 Oct 2017, Jason Ekstrand wrote:<br>
> On October 16, 2017 4:18:45 PM Chad Versace <<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>> wrote:<br>
><br>
> ><br>
> > Add missing close(fd) for case<br>
> > VK_EXTERNAL_SEMAPHORE_HANDLE_<wbr>TYPE_OPAQUE_FD_BIT_KHR,<br>
> > subcase ANV_SEMAPHORE_TYPE_BO.<br>
> > ---<br>
> >  src/intel/vulkan/anv_queue.c | 22 +++++++++++-----------<br>
> >  1 file changed, 11 insertions(+), 11 deletions(-)<br>
> ><br>
> > diff --git a/src/intel/vulkan/anv_queue.c b/src/intel/vulkan/anv_queue.c<br>
> > index e26254a87e..180c907781 100644<br>
> > --- a/src/intel/vulkan/anv_queue.c<br>
> > +++ b/src/intel/vulkan/anv_queue.c<br>
> > @@ -1020,17 +1020,6 @@ VkResult anv_ImportSemaphoreFdKHR(<br>
> >           new_impl.syncobj = anv_gem_syncobj_fd_to_handle(<wbr>device, fd);<br>
> >           if (!new_impl.syncobj)<br>
> >              return vk_error(VK_ERROR_INVALID_<wbr>EXTERNAL_HANDLE_KHR);<br>
> > -<br>
> > -         /* From the Vulkan spec:<br>
> > -          *<br>
> > -          *    "Importing semaphore state from a file descriptor transfers<br>
> > -          *    ownership of the file descriptor from the application to the<br>
> > -          *    Vulkan implementation. The application must not perform any<br>
> > -          *    operations on the file descriptor after a successful import."<br>
> > -          *<br>
> > -          * If the import fails, we leave the file descriptor open.<br>
> > -          */<br>
> > -         close(pImportSemaphoreFdInfo-><wbr>fd);<br>
> >        } else {<br>
> >           new_impl.type = ANV_SEMAPHORE_TYPE_BO;<br>
> ><br>
> > @@ -1044,6 +1033,17 @@ VkResult anv_ImportSemaphoreFdKHR(<br>
> >            */<br>
> >           assert(!(new_impl.bo->flags & EXEC_OBJECT_ASYNC));<br>
> >        }<br>
> > +<br>
> > +      /* From the Vulkan spec:<br>
> > +       *<br>
> > +       *    "Importing semaphore state from a file descriptor transfers<br>
> > +       *    ownership of the file descriptor from the application to the<br>
> > +       *    Vulkan implementation. The application must not perform any<br>
> > +       *    operations on the file descriptor after a successful import."<br>
> > +       *<br>
> > +       * If the import fails, we leave the file descriptor open.<br>
> > +       */<br>
> > +      close(fd);<br>
><br>
> Indentation looks off here.  Other than that, LGTM<br>
<br>
</div></div>The indentation is intended. The expanded hunk looks like this. The<br>
close() catches both branches of the if-tree.<br>
<br>
    case VK_EXTERNAL_SEMAPHORE_HANDLE_<wbr>TYPE_OPAQUE_FD_BIT_KHR:<br>
        if (has_syncobj) {<br>
            ...<br>
            anv_gem_syncobj_fd_to_handle(<wbr>dev, fd);<br>
            if (fail)<br>
                return error;<br>
        } else {<br>
            ...<br>
            anv_bo_cache_import_with_size(<wbr>dev, fd, ...);<br>
            if (fail)<br>
                return error;<br>
        }<br>
<br>
        close(fd);<br></blockquote><div> </div></div></div><div class="gmail_extra">Yes, but it looked, in patchwork, like it was indented 1 space too far.<br></div></div>