From reviews-return-31641-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Tue Apr 19 22:21:56 2016 Return-Path: X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id D8DC0196AE for ; Tue, 19 Apr 2016 22:21:56 +0000 (UTC) Received: (qmail 29451 invoked by uid 500); 19 Apr 2016 22:21:51 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 29420 invoked by uid 500); 19 Apr 2016 22:21:51 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 29397 invoked by uid 99); 19 Apr 2016 22:21:51 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 19 Apr 2016 22:21:51 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 53BE72B17E8; Tue, 19 Apr 2016 22:21:47 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============0791693758175303393==" MIME-Version: 1.0 Subject: Re: Review Request 43985: Windows: [1/3] Implemented `sendfile`. From: Michael Park To: Joris Van Remoortere , Artem Harutyunyan , Michael Park , Alex Clemmer Cc: Daniel Pravat , mesos Date: Tue, 19 Apr 2016 22:21:47 -0000 Message-ID: <20160419222147.8130.29383@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Michael Park X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/43985/ X-Sender: Michael Park References: <20160416011045.21369.49648@reviews.apache.org> In-Reply-To: <20160416011045.21369.49648@reviews.apache.org> Reply-To: Michael Park X-ReviewRequest-Repository: mesos --===============0791693758175303393== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43985/#review129633 ----------------------------------------------------------- 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/sendfile.hpp (lines 33 - 34) (1) `s/On error, On error,/On error,/` (2) Remove the extra space in `// Try`. i.e., `// Try` 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/sendfile.hpp (lines 39 - 40) ``` inline Try sendfile( int s, int fd, off_t offset, size_t length) { ``` 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/sendfile.hpp (line 45) `s/send/sent/` 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/sendfile.hpp (line 46) (1) `s/return return/return/` (2) Based on other syscalls, it seems like we should just do `return SocketError();`. e.g., `close.hpp`, `chroot.hpp`, `fnctl.hpp`, etc. Here and below. 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp (lines 16 - 17) When we fork to have separate implementation for `POSIX` and `Windows`, we should keep them completely separate. That is, even though `` is used in both implementations, they should include them individually. For example, starting from `3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/sendfile.hpp` we can't find where `ErrnoError` came from. 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp (lines 19 - 24) Let's consolidate this with the `process::network::SocketError` in `3rdparty/libprocess/include/process/network.hpp`. For now, I think `stout/error.hpp` is the best place to put it. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (lines 16 - 17) Add a newline in between. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (line 23) Backquotes around `Try`. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (lines 24 - 25) ``` inline Try sendfile( int s, int fd, off_t offset, size_t length) { ``` 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (line 27) `s/ will/, it will/` 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (lines 28 - 30) Please use the correct C++ casts here for the casts to `HANDLE`, `LONG`. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (line 30) `hight_Part`? did we mean `high_part`? Just `high` seems fine too. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (line 31) It looks like the second parameter of `SetFilePointer` is `LONG`, not `DWORD`. Why do we cast to `DWORD`? Also, please use C++ cast. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (line 33) Should return `SocketError` here (I know they're the same, but still). 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp (line 39) Should return `SocketError` here (I know they're the same, but still). - Michael Park On April 16, 2016, 1:10 a.m., Daniel Pravat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43985/ > ----------------------------------------------------------- > > (Updated April 16, 2016, 1:10 a.m.) > > > Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van Remoortere, and Michael Park. > > > Repository: mesos > > > Description > ------- > > Windows: [1/3] Implemented `sendfile`. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/sendfile.hpp 293f82f6730551491504721e0af28e9537540db1 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp f3197679a9c22b37acae003262e5c6d21e110f56 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp 750ca6749cb028703ed2fb5dec2aac6c5dabea0d > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp a7f855dc9d0a87fe3b6d1611e7ae22e4d7cd7b6d > > Diff: https://reviews.apache.org/r/43985/diff/ > > > Testing > ------- > > Build Windows,OSX > > > Thanks, > > Daniel Pravat > > --===============0791693758175303393==--