mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Clemmer" <clemmer.alexan...@gmail.com>
Subject Re: Review Request 39019: Windows: Added dirent compat code for non-Unix systems.
Date Mon, 11 Jan 2016 10:34:44 GMT


> On Jan. 9, 2016, 12:42 a.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp, lines
61-62
> > <https://reviews.apache.org/r/39019/diff/10/?file=1175210#file1175210line61>
> >
> >     I don't see the check for pathSize wrt. MAX_PATH.

You're right that this should be there, just because it is good defensive style.

But, just for my own education: do you see a problem that I do not? There are known-problem
overflows (like the calls to `strcat`), but they are innate to the API, because we don't know
the length of `path` ahead of time, and therefore they don't go away if we add a `MAX_PATH`
check. Or perhaps I am missing something here?


> On Jan. 9, 2016, 12:42 a.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp, line
214
> > <https://reviews.apache.org/r/39019/diff/10/?file=1175210#file1175210line214>
> >
> >     are `null-terminated` right?
> >     Same for the commeint in `_reentrantAdvanceDirStream`

Good question. The point I'm actually trying to make is that `directory->curr.d_name` can
be a statically-sized array of `MAX_PATH` size because `directory->fd.cFileName` is also
a statically sized array of `MAX_PATH` size.

Let's update the comments so that this is more clear.


> On Jan. 9, 2016, 12:42 a.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp, lines
193-199
> > <https://reviews.apache.org/r/39019/diff/10/?file=1175210#file1175210line193>
> >
> >     `free` is meant to be safe for `NULL/nullptr`.
> >     Why not follow this pattern?
> >     asserting `directory != NULL` seems like it might surprise people?

Well, you are the C++ programmer, so I'll trust your judgement on the `assert`s here.

But, since this question about `assert` comes up 3 times in your review, I'll answer your
question, and justify the switch. I initially chose to do it this way because I think internal
APIs are easier to reason about if all the functions assume the data in non-`_OUT_` params
is fully initialized before you call. I just used `assert` to make this assumption explicit,
which I think is a good way of broadcasting to internal users what the contract is.

Of course, (1) if you think this is less clear, then my thesis is clearly wrong, and (2) the
actual difference in this case is not really a substantial amount of code/complexity, so the
cost of switching to your model is not prohibitive, so if I were to advocate staying it would
be for purely religious reasons.

Therefore, we shall transition to fit your suggestion.


- Alex


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39019/#review113576
-----------------------------------------------------------


On Dec. 23, 2015, 6:44 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39019/
> -----------------------------------------------------------
> 
> (Updated Dec. 23, 2015, 6:44 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere,
and Joseph Wu.
> 
> 
> Bugs: MESOS-3441
>     https://issues.apache.org/jira/browse/MESOS-3441
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Windows: Added dirent compat code for non-Unix systems.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe

>   3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp PRE-CREATION

>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp d46e262e0fd1c2de36f3bf19d8bd693c23bf58cd

> 
> Diff: https://reviews.apache.org/r/39019/diff/
> 
> 
> Testing
> -------
> 
> `make check` from autotools on Ubuntu 15.
> `make check` from CMake on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message