mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 54952: Added EINVAL to the list of expected `getpwnam_r()` errors.
Date Thu, 22 Dec 2016 18:50:15 GMT

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



I'm not quite sure the error processing logic of the functions you touch in this patch is
correct. According to [POSIX documentation](http://pubs.opengroup.org/onlinepubs/009695399/functions/getpwnam.html),
`errno` is set only if an error occurs, which does not include absent entries. However this
seems [not to be the case in Linux](https://linux.die.net/man/3/getpwnam_r) and apparently
that's why a special error handling section was introduced in https://reviews.apache.org/r/24249/.

According to the documentation, here is how the workflow should look like (you can group it
into nested `if` statements):
```
if (retcode == 0 && result != nullptr) {
  // Matching entry found.
  uid_t uid = passwd.pw_uid;
  delete[] buffer;
  return uid;
} else if (retcode == 0 && result == nullptr) {
  // Matching entry not found.
  delete[] buffer;
  return None();
} else if (retcode != 0 && errno == ERANGE) {
  // Buffer is not large enough.
  size *= 2;
  delete[] buffer;
} else if (retcode != 0 &&
           (errno == EINTR ||
            errno == EIO ||
            errno == EMFILE ||
            errno == ENFILE ||
            errno == ENOMEM)) {
  // Non-transient failure.
  delete[] buffer;
  return ErrnoError("Failed to get username information");
} else {
  // Some Linux distributions...
  // Means the entry is not found.
  delete[] buffer;
  return None();
}

...
```

It seems that fixing the overall workflow is a more robust solution than adding error messages
one by one each time the C library is updated.

- Alexander Rukletsov


On Dec. 21, 2016, 10:38 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54952/
> -----------------------------------------------------------
> 
> (Updated Dec. 21, 2016, 10:38 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-6826
>     https://issues.apache.org/jira/browse/MESOS-6826
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> On recent Arch Linux, `getpwnam_r` can return EINVAL when passed an
> invalid user name, which caused the `OsTest.User` test to fail.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/os/posix/su.hpp f3f45ebf006f0f8e7e70b0f4c4ed76465530c58e

> 
> Diff: https://reviews.apache.org/r/54952/diff/
> 
> 
> Testing
> -------
> 
> `make check` on Arch Linux. `OsTest.User` fails without this patch but succeeds with
this patch.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


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