This is the mail archive of the cygwin-patches mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] posix_fadvise() *returns* error codes but does not set errno


On Thu, Nov 2, 2017 at 3:58 PM, Corinna Vinschen
<corinna-cygwin@cygwin.com> wrote:
> Hi Eric,
>
> On Nov  2 14:26, Erik M. Bray wrote:
>> Also updates the fhandler_*::fadvise implementations to adhere to the same
>> semantics.
>
> Good catch.  I have just some style nits.
>
>> ---
>>  winsup/cygwin/fhandler.cc           |  3 +--
>>  winsup/cygwin/fhandler_disk_file.cc | 16 ++++++----------
>>  winsup/cygwin/pipe.cc               |  3 +--
>>  winsup/cygwin/syscalls.cc           |  2 +-
>>  4 files changed, 9 insertions(+), 15 deletions(-)
>>
>> diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
>> index d719b7c..858c1fd 100644
>> --- a/winsup/cygwin/fhandler.cc
>> +++ b/winsup/cygwin/fhandler.cc
>> @@ -1764,8 +1764,7 @@ fhandler_base::fsetxattr (const char *name, const void *value, size_t size,
>>  int
>>  fhandler_base::fadvise (off_t offset, off_t length, int advice)
>>  {
>> -  set_errno (EINVAL);
>> -  return -1;
>> +  return EINVAL;
>>  }
>>
>>  int
>> diff --git a/winsup/cygwin/fhandler_disk_file.cc b/winsup/cygwin/fhandler_disk_file.cc
>> index 2144a4c..f46e355 100644
>> --- a/winsup/cygwin/fhandler_disk_file.cc
>> +++ b/winsup/cygwin/fhandler_disk_file.cc
>> @@ -1076,8 +1076,7 @@ fhandler_disk_file::fadvise (off_t offset, off_t length, int advice)
>>  {
>>    if (advice < POSIX_FADV_NORMAL || advice > POSIX_FADV_NOREUSE)
>>      {
>> -      set_errno (EINVAL);
>> -      return -1;
>> +      return EINVAL;
>>      }
>
> Please remove the braces for a one-line block.
>
>>
>>    /* Windows only supports advice flags for the whole file.  We're using
>> @@ -1097,21 +1096,18 @@ fhandler_disk_file::fadvise (off_t offset, off_t length, int advice)
>>    NTSTATUS status = NtQueryInformationFile (get_handle (), &io,
>>                                           &fmi, sizeof fmi,
>>                                           FileModeInformation);
>> -  if (!NT_SUCCESS (status))
>> -    __seterrno_from_nt_status (status);
>> -  else
>> +  if (NT_SUCCESS (status))
>>      {
>>        fmi.Mode &= ~FILE_SEQUENTIAL_ONLY;
>>        if (advice == POSIX_FADV_SEQUENTIAL)
>> -     fmi.Mode |= FILE_SEQUENTIAL_ONLY;
>> +        fmi.Mode |= FILE_SEQUENTIAL_ONLY;
>
> You changed indentation here for no apparent reason (TABs vs spaces).

Sorry, it's just that the indentation in this function is already an
unholy mess of tabs vs spaces from line to line.  This was just my
editor's (ill-advised) attempt to make it legible.  I can remove the
unrelated whitespace changes from the patch.

>>        status = NtSetInformationFile (get_handle (), &io, &fmi, sizeof fmi,
>> -                                  FileModeInformation);
>> +                                     FileModeInformation);
>>        if (NT_SUCCESS (status))
>> -     return 0;
>> -      __seterrno_from_nt_status (status);
>> +         return 0;
>>      }
>
> Ditto and ditto.
>
>> -  return -1;
>> +  return geterrno_from_nt_status (status);
>>  }
>>
>>  int
>> diff --git a/winsup/cygwin/pipe.cc b/winsup/cygwin/pipe.cc
>> index 79b7966..8738d34 100644
>> --- a/winsup/cygwin/pipe.cc
>> +++ b/winsup/cygwin/pipe.cc
>> @@ -165,8 +165,7 @@ fhandler_pipe::lseek (off_t offset, int whence)
>>  int
>>  fhandler_pipe::fadvise (off_t offset, off_t length, int advice)
>>  {
>> -  set_errno (ESPIPE);
>> -  return -1;
>> +  return ESPIPE;
>>  }
>>
>>  int
>> diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
>> index caa3a77..d0d735b 100644
>> --- a/winsup/cygwin/syscalls.cc
>> +++ b/winsup/cygwin/syscalls.cc
>> @@ -2937,7 +2937,7 @@ posix_fadvise (int fd, off_t offset, off_t len, int advice)
>>    if (cfd >= 0)
>>      res = cfd->fadvise (offset, len, advice);
>>    else
>> -    set_errno (EBADF);
>> +    res = EBADF;
>>    syscall_printf ("%R = posix_fadvice(%d, %D, %D, %d)",
>>                 res, fd, offset, len, advice);
>>    return res;
>> --
>> 2.8.3
>
> Other than that, looks good.
>
>
> Thanks,
> Corinna
>
> --
> Corinna Vinschen                  Please, send mails regarding Cygwin to
> Cygwin Maintainer                 cygwin AT cygwin DOT com
> Red Hat


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]