mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Re: Please use `int_fd` instead of `int` for file descriptors
Date Fri, 01 Dec 2017 01:58:23 GMT
Unfortunately the root of our problem is that the cross-platform review 
bots are still not up-to-snuff. Most people (including myself) won't 
wait around for a bot that takes sometimes 24 hours to review the patch 
after an update. So, while increasing the discoverability of `int_fd` I 
think will help, _really_ we need patches to be shown as broken on 
Windows (or elsewhere) before they're committed.

Off the top of my head, we've fixed a Windows build break due to `int` 
vs `int_fd` at least four times, which is why I decided to send out a 
heads-up email. But ideally the we hunker down and figure out how to 
make the bots post build results much more quickly. I think it needs to 
be under an hour for people to start paying attention to them.

> Yes, we have code using int_fd within hashmaps, maps, etc, already 
> across
> platforms so I assume it has the properties I listed on windows, but it
> would be good to document that as being something that int_fd is 
> guaranteed
> to provide.

I agree, I'll get this documented. I think this probably extends to some 
other cross-platform abstractions too, I'll do a pass and see what I can 
do.

On 11/30/2017 5:47 pm, Michael Park wrote:
> I agree it would be nice to document
> the supported operations on an `int_fd`.
> 
> I'm not sure how to actually help with
> the issue of making `int_fd` more
> discoverable. The only idea I've got is
> a ClangTidy check to complain about
> variables of type `int` named `fd` and
> other similar common names for file
> descriptors such as `socket`.
> 
> Thanks,
> 
> MPark
> On Thu, Nov 30, 2017 at 4:41 PM Benjamin Mahler <bmahler@apache.org> 
> wrote:
> 
>> On Thu, Nov 30, 2017 at 11:12 PM, Andrew Schwartzmeyer <
>> andrew@schwartzmeyer.com> wrote:
>> 
>> > For Linux it is literally an `int`:
>> >
>> > using int_fd = int;
>> >>
>> >
>> > https://github.com/apache/mesos/blob/bf4bc6380cb99132736fbbe
>> > fc85f3a7ca60b032c/3rdparty/stout/include/stout/os/int_fd.hpp#L35
>> >
>> >
>> Yes I realize that :)
>> 
>> 
>> > So it's a safe drop-in replacement on non-Windows platforms, with all the
>> > same properties of an `int`.
>> >
>> > It's only on Windows where it is instead `os::WindowsFD`, and then you
>> may
>> > need to worry about its properties and semantics. Do you want these
>> > documented in `stout/os/windows/fd.hpp`?
>> 
>> 
>> Yes, we have code using int_fd within hashmaps, maps, etc, already 
>> across
>> platforms so I assume it has the properties I listed on windows, but 
>> it
>> would be good to document that as being something that int_fd is 
>> guaranteed
>> to provide.
>> 
>> 
>> >
>> >
>> > On 11/30/2017 3:05 pm, Benjamin Mahler wrote:
>> >
>> >> Is it possible to document in that header the properties of int_fd that
>> we
>> >> can rely on?
>> >>
>> >> For example, it has a hash defined for use in unordered map, set, etc.
>> >> It's
>> >> a POD type, etc.
>> >>
>> >> On Wed, Nov 29, 2017 at 10:17 PM, Andrew Schwartzmeyer <
>> >> andrew@schwartzmeyer.com> wrote:
>> >>
>> >> Hello everyone!
>> >>>
>> >>> I've realized that a lot of developers working in libprocess (and
>> >>> elsewhere) may not know about how to handle file descriptors in a
>> >>> cross-platform way for Mesos.
>> >>>
>> >>> IMPORTANT: You cannot just use `int`. File descriptors on Windows are
>> >>> various types of handles, but not just an `int`.
>> >>>
>> >>> The abstraction we use in Mesos is `int_fd`, found here:
>> >>> https://github.com/apache/mesos/blob/master/3rdparty/stout/
>> >>> include/stout/os/int_fd.hpp
>> >>>
>> >>> On non-Windows platforms, it's just an `int`. But on Windows, it's a
>> >>> `WindowsFD` which can be an `int` (from the Windows CRT which we're
>> >>> deprecating), a `HANDLE` (the Windows 32 API), and a `SOCKET` (from
the
>> >>> WinSock library). If you're curious, the implementation is here:
>> >>> https://github.com/apache/mesos/blob/master/3rdparty/stout/
>> >>> include/stout/os/windows/fd.hpp
>> >>>
>> >>> I just want you to be aware that if you're writing code and need an
>> `int`
>> >>> file descriptor, please use `int_fd` (and include the appropriate
>> header)
>> >>> instead of `int`, as otherwise you break the Windows build.
>> >>>
>> >>> Thank you,
>> >>>
>> >>> Andy
>> >>>
>> >>>
>> 

Mime
View raw message