<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_RESOLVED bz_closed"
title="RESOLVED FIXED - [UNIX] Simplify the file descriptor handling in SharedMemory"
href="https://bugs.webkit.org/show_bug.cgi?id=144046#c5">Comment # 5</a>
on <a class="bz_bug_link
bz_status_RESOLVED bz_closed"
title="RESOLVED FIXED - [UNIX] Simplify the file descriptor handling in SharedMemory"
href="https://bugs.webkit.org/show_bug.cgi?id=144046">bug 144046</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=144046#c3">comment #3</a>)
<span class="quote">> (In reply to <a href="show_bug.cgi?id=144046#c2">comment #2</a>)
> > Comment on <span class=""><a href="attachment.cgi?id=251329&action=diff" name="attach_251329" title="Patch">attachment 251329</a> <a href="attachment.cgi?id=251329&action=edit" title="Patch">[details]</a></span>
> > Patch
> >
> > View in context:
> > <a href="https://bugs.webkit.org/attachment.cgi?id=251329&action=review">https://bugs.webkit.org/attachment.cgi?id=251329&action=review</a>
> >
> > Looks OK.
>
> Thanks for the review.
>
> > > Source/WebKit2/Platform/IPC/Attachment.h:59
> > > + Attachment(const Attachment&) = default;
> > > + Attachment& operator=(Attachment&) = default;
> >
> > I think you want = delete here, not = default. It doesn’t seem OK to just
> > copy a file descriptor!
>
> Yes, I first tried to make the class non copyable, and release the fd in the
> destructor to get rid of the dispose() method and the explicit attachment
> dispose made by the ArgumentEncoder, ArgumentDecoder and ConnectionUnix. But
> it was not possible, or I didn't find the way. Encoding/decoding attachments
> is adding them to the encoder/decoder class that take the ownership of the
> file descriptors until they are passed to the platform specific send/receive
> IPC methods. The thing is that Attachment::encode() is const and does
> encoder.addAttachment(*this); Even if we change
> ArgumentEncoder::addAttachment to receive an Attachment&& we can't
> WTF::move(*this) in the const encode method. I'm not a C++ expert, so maybe
> there's a way. In the end I forgot about that part and left the explicit
> dispose, but avoiding the manual fd = -1 in several places and renaming the
> confusing adoptFrom, releaseTo methods.</span >
Zan suggested to make the copy constructor and assignment private and use it only in ::encode() doing encoder.addAttachment(Attachment(*this)); That way we can ensure that any public assignment is a move. We also need to make the file descriptor member mutable and explicitly set to -1 in ::encode() since the ownership is transfered to the encoder. It's tricky, but it works. I'll submit a new bug.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>