[webkit-reviews] review granted: [Bug 238726] [Unix] Add UnixFileDescriptor, use it in IPC::Semaphore : [Attachment 456681] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 238726: [Unix] Add UnixFileDescriptor, use it in IPC::Semaphore
https://bugs.webkit.org/show_bug.cgi?id=238726

Attachment 456681: Patch

https://bugs.webkit.org/attachment.cgi?id=456681&action=review




--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 456681
  --> https://bugs.webkit.org/attachment.cgi?id=456681
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456681&action=review

> Source/WTF/wtf/unix/UnixFileDescriptor.h:51
> +    enum DuplicationTag { Duplicate };
> +    UnixFileDescriptor(int fd, DuplicationTag)
> +    {
> +	   if (fd >= 0)
> +	       m_value = dupCloseOnExec(fd);
> +    }

I think we can keep this private for now, since it's always used from
::duplicate().

> Source/WTF/wtf/unix/UnixFileDescriptor.h:88
> +

using WTF::UnixFileDescriptor

> Source/WebKit/Platform/IPC/unix/IPCSemaphoreUnix.cpp:44
> +    m_fd = { eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK | EFD_SEMAPHORE),
WTF::UnixFileDescriptor::Adopt };

I still prefer m_fd = adoptFileDescriptor(eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK
| EFD_SEMAPHORE)); but no strong opinion if you really want to expose
WTF::UnixFileDescriptor::Adopt and WTF::UnixFileDescriptor::Duplicate

> Source/WebKit/Shared/WebCoreArgumentCoders.h:854
> +#if USE(UNIX_DOMAIN_SOCKETS)
> +
> +template<> struct ArgumentCoder<WTF::UnixFileDescriptor> {
> +    static void encode(Encoder&, const WTF::UnixFileDescriptor&);
> +    static void encode(Encoder&, WTF::UnixFileDescriptor&&);
> +    static std::optional<WTF::UnixFileDescriptor> decode(Decoder&);
> +};
> +
> +#endif

This doesn't really belong here, since UnixFileDescriptor is not a WebCore
class. Why don't you add the encode/decode methods to UnixFileDescriptor class?