Skip to content

Refactoring std os setenv#14736

Closed
darnuria wants to merge 4 commits intorust-lang:masterfrom
darnuria:refactoring_std_os_setenv
Closed

Refactoring std os setenv#14736
darnuria wants to merge 4 commits intorust-lang:masterfrom
darnuria:refactoring_std_os_setenv

Conversation

@darnuria
Copy link
Contributor

@darnuria darnuria commented Jun 7, 2014

Hello!

After talking with @huonw on Irc and trying different thing, I purpose to add a error handling over this wrapper of libc::setenv for catching errors.

I will need some help on the two commit marked as fixup!. I am still a newbie to rust. :)

huonw
huonw reviewed Jun 8, 2014
View reviewed changes
src/compiletest/compiletest.rs Outdated
Copy link
Contributor

@huonw huonw Jun 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be better as fail!("setenv failed: {}", e).

@alexcrichton
Copy link
Member

alexcrichton commented Jun 8, 2014

The only documented error case on unix for setenv is ENOMEM. We currently abort on OOM everywhere else in Rust, so it seems a little odd to start worrying about it for this one function (and getting annoying errors about checking the result of setenv).

Are there error cases for windows that we're worried about not catching? If it's just OOM then I'd tend to think that these functions should continue to return ()

@darnuria
Copy link
Contributor Author

darnuria commented Jun 8, 2014

@alexcrichton: Funny on my Linux I have two errors documented.

man setenv on Ubuntu 14.04:

RETURN VALUE
       The setenv() function returns zero on success, or  -1  on  error,  with
       errno set to indicate the cause of the error.
       The  unsetenv()  function returns zero on success, or -1 on error, with
       errno set to indicate the cause of the error.
ERRORS
       EINVAL name is NULL, points to a string of length 0, or contains an '='
              character.
       ENOMEM Insufficient memory to add a new variable to the environment.
CONFORMING TO
       4.3BSD, POSIX.1-2001.
BUGS
       POSIX.1-2001  specifies  that  if  name contains an '=' character, then
       setenv() should fail with the error EINVAL; however, versions of  glibc
       before 2.3.4 allowed an '=' sign in name.

And rustc is compiled with:

GNU C Library (Ubuntu EGLIBC 2.19-0ubuntu6) stable release version 2.19, by Roland McGrath et al.
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.
Compiled by GNU CC version 4.8.2.
Compiled on a Linux 3.13.9 system on 2014-04-12.
Available extensions:
    crypt add-on version 2.1 by Michael Glad and others
    GNU Libidn by Simon Josefsson
    Native POSIX Threads Library by Ulrich Drepper et al
    BIND-8.2.3-T5B
libc ABIs: UNIQUE IFUNC
For bug reporting instructions, please see:
<https://bugs.launchpad.net/ubuntu/+source/eglibc/+bugs>.

@alexcrichton
Copy link
Member

alexcrichton commented Jun 8, 2014

Ah yes, I also had EINVAL as an error listed, but we fail pretty much everywhere else in std::os if a string contains NUL, and I believe that this should remain consistent with the other methods.

@darnuria
Copy link
Contributor Author

darnuria commented Jun 8, 2014

So the best solution is to:

  • Drop this pull-request?
  • Rewrite some std::os with OsError/IoError in mind?

If somebody wants to write something like a shell in Rust it would be nice to have the possibility to check errors like theses instead of having a internal error.

@alexcrichton
Copy link
Member

alexcrichton commented Jun 8, 2014

I would have to look at other functions as well, but failing on NUL is pretty common throughout the codebase, and it may be intentional rather than accidental. For now though, I'm going to close this pending further investigation.

@darnuria
Copy link
Contributor Author

darnuria commented Jun 8, 2014

@alexcrichton https://gist.github.com/darnuria/9312f285ee8b265abf92
Originally I worked on this, because of this case.

@alexcrichton
Copy link
Member

alexcrichton commented Jun 9, 2014

That is an interesting case!

flip1995 added a commit to flip1995/rust that referenced this pull request May 15, 2025
@flip1995
Read PR body from event in clippy_changelog action (rust-lang#14736)
f60d7f9 
Simplifies the implementation a bit, also makes the message an error
annotation

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@darnuria @alexcrichton @huonw