Added parser for `/proc/[pid]/environ` #28

mexus · 2017-08-26T22:11:51Z

Hi!

First of all, thanks for a great and simple library!
I though it'd be nice to contribute to the project so I found out that there is no parser for a /proc/[pid]/environ file and implemented it :)

Please consider to review & merge if it's okay.

danburkert

In terms of the API - what do you think about modeling it after VarOs, an iterator of (OsString, OsString)?

danburkert · 2017-08-26T23:17:07Z

src/pid/environ.rs

+/// Parses the provided environ file.
+fn environ_path<P: AsRef<Path>>(path: P) -> Result<BTreeMap<Vec<u8>, Vec<u8>>> {
+    let file_size = metadata(&path)?.len();
+    let mut buf = vec![0u8; file_size as usize];


This is racy with updates to the env, right? I think it would be better and simpler just to use File::read_to_end.

From man 5 proc:

If, after an execve(2), the process modifies its environment (e.g., by calling functions such as putenv(3) or modifying the environ(7) variable directly), this file will not reflect those changes.

So the only race condition I can think of is the process is closed in between getting the size and reading the file.. But still you are right, will fix.

mexus · 2017-08-26T23:23:09Z

I totally forgot about VarOs! Shame on me, shame on me..

mexus · 2017-08-27T00:20:51Z

Updated the patch. Unfortunately VarOs can't be built but with std::env::vars_os :(. So I went with Vec<(OsString, OsString)>.

danburkert · 2017-08-27T00:37:02Z

Sorry for not being clear, I was suggesting creating a new type like std::env::VarOs (it could even be named procinfo::environ::VarOs). The advantage of an iterator is that it can be collected into a vec or a map easily.

danburkert · 2017-08-27T00:39:43Z

actually in terms of naming I think either of these would be fine:

procinfo::pid::Environ
procinfo::pid::VarOs

each has advantages, what do you think?

mexus · 2017-08-27T00:52:21Z

Ah, np. Personally I would rather go with procinfo::pid::Environ since it sounds easier to guess what it is (IMHO), but I'd say the choice is yours.
Btw, regardless of the naming, do you want it to be a full-featured struct or just a typedef? I don't see any advantages with a separate class but there might be some I just can't think of.

danburkert · 2017-08-27T01:03:17Z

Environ sounds good. In terms of struct vs typedef I think the only option will be struct since you can typedef a type containing a closure, but if you have a trick in unaware of to make that possible then go for it (but no boxing please).

…

On Sat, Aug 26, 2017 at 5:52 PM Denis ***@***.***> wrote: Ah, np. Personally I would rather go with procinfo::pid::Environ since it sounds easier to guess what it is (IMHO), but I'd say the choice is yours. Btw, regardless of the naming, do you want it to be a full-featured struct or just a typedef? I don't see any advantages with a separate class but there might be some I just can't think of. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#28 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJx8miZMi9cA7KZawmTnH2J9KnOsnKTks5scL3FgaJpZM4PDooe> .

danburkert · 2017-08-27T01:03:43Z

Can't *

…

On Sat, Aug 26, 2017 at 6:03 PM Dan Burkert ***@***.***> wrote: Environ sounds good. In terms of struct vs typedef I think the only option will be struct since you can typedef a type containing a closure, but if you have a trick in unaware of to make that possible then go for it (but no boxing please). On Sat, Aug 26, 2017 at 5:52 PM Denis ***@***.***> wrote: > Ah, np. Personally I would rather go with procinfo::pid::Environ since > it sounds easier to guess what it is (IMHO), but I'd say the choice is > yours. > Btw, regardless of the naming, do you want it to be a full-featured > struct or just a typedef? I don't see any advantages with a separate > class but there might be some I just can't think of. > > — > You are receiving this because you commented. > > > Reply to this email directly, view it on GitHub > <#28 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAJx8miZMi9cA7KZawmTnH2J9KnOsnKTks5scL3FgaJpZM4PDooe> > . >

mexus · 2017-08-27T01:10:53Z

Well, I though about a simple type Environ = Vec<(OsString, OsString)>. Do you think it won't suffice? It seems to be working fine…

danburkert · 2017-08-27T01:11:24Z

On second thought that was gibberish. Up to you on typedef vs struct.

…

On Sat, Aug 26, 2017 at 6:03 PM Dan Burkert ***@***.***> wrote: Can't * On Sat, Aug 26, 2017 at 6:03 PM Dan Burkert ***@***.***> wrote: > Environ sounds good. In terms of struct vs typedef I think the only > option will be struct since you can typedef a type containing a closure, > but if you have a trick in unaware of to make that possible then go for it > (but no boxing please). > > On Sat, Aug 26, 2017 at 5:52 PM Denis ***@***.***> wrote: > >> Ah, np. Personally I would rather go with procinfo::pid::Environ since >> it sounds easier to guess what it is (IMHO), but I'd say the choice is >> yours. >> Btw, regardless of the naming, do you want it to be a full-featured >> struct or just a typedef? I don't see any advantages with a separate >> class but there might be some I just can't think of. >> >> — >> You are receiving this because you commented. >> >> >> Reply to this email directly, view it on GitHub >> <#28 (comment)>, >> or mute the thread >> <https://github.com/notifications/unsubscribe-auth/AAJx8miZMi9cA7KZawmTnH2J9KnOsnKTks5scL3FgaJpZM4PDooe> >> . >> >

mexus · 2017-08-27T01:17:44Z

Anything else? :)

danburkert · 2017-08-27T02:55:07Z

src/pid/environ.rs

+use libc::pid_t;
+
+/// A list of environment variables and their values.
+pub type Environ = Vec<(OsString, OsString)>;


I was hoping there was a way to avoid collecting into an intermediate Vec, but I spent a while on it and didn't find an elegant way. Since the API's just exposing a vec, I think we should skip the type def and just return Vec<(OsString, OsString)>.

Hm. I'll think a bit more on this.

danburkert · 2017-08-27T02:56:33Z

src/pid/environ.rs

+    // Parse 'key=value" pair.
+    named!(
+        pair<&[u8], (&[u8], &[u8])>,
+        tuple!(take_until_and_consume!("="), take_until!("\0"))


This is going to have different behavior that the standard library when the key contains a leading '=' character (https://github.com/rust-lang/rust/blob/1.19.0/src/libstd/sys/unix/os.rs#L405-L408). Can this skip a character first to avoid this?

Also, please add a test case for this.

Not quite sure where did they get that "strategy" from. Let's say setenv function explicitly checks that the name doesn't have an equal sign inside: https://github.com/lattera/glibc/blob/a2f34833b1042d5d8eeb263b4cf4caaea138c4ad/stdlib/setenv.c#L301

Also https://stackoverflow.com/a/2821183/1449426 has nice answers with links and everything on the matter.

While it's definitely possible to support such a behaviour it will make the code to be more complex, and my humble opinion is that one should avoid unnecessary code complication.

Nonetheless it appears to be a real issue. On my up-to-date linux machine (kernel 4.11) the output of env =FOO=BAR xxd /proc/self/environ shows that leading = characters can appear.

danburkert · 2017-08-27T02:57:33Z

src/pid/environ.rs

+    );
+    multi(input).map(|r| {
+        r.into_iter()
+            .map(|(x, y)| {


If you move this map call into the pair function, the number of vectors constructed can be reduced from two to one.

Mm, I have there a tuple of two values. How can I go through them with a single call of the map? Although it definitely makes sense to apply the map right when the values are obtained (it makes the code to be more clear indeed).

The full code context has disappeared (thanks GitHub), but I think the idea was that you could use a map! call inside the parsing code, instead of the Iterator::map call here.

mexus · 2017-08-27T11:46:26Z

Updated the patch again. This time I used the streams (Iterator) concept.

mexus · 2017-08-27T12:03:58Z

Oh man that old rust compiler makes me sick

danburkert · 2017-08-27T18:26:41Z

Oh man that old rust compiler makes me sick

Not sure what you mean by this, I was debating whether to ask you just to copy that code wholesale. The memchr solution is going to be significantly faster, but it's probably not worth bringing in a dependency for.

Updated the patch again. This time I used the streams (Iterator) concept.

The new code is allocating more vectors than the previous version, one for each entry.

mexus · 2017-08-27T19:15:00Z

The new code is allocating more vectors than the previous version, one for each entry

Yeah, you're totally right, I've overlooked it somehow :(

Okay, seems like I have to put some (probably a lot of) thought into the matter.

danburkert · 2017-08-27T21:30:07Z

As I mentioned, I thought your previous cut was very close. Here's the change from .map to map! which I was hinting at:

/// Parses the provided buffer.
fn parse_environ(input: &[u8]) -> IResult<&[u8], Vec<(OsString, OsString)>> {
    // Parse 'key=value" pair.
    named!(pair<&[u8], (OsString, OsString)>,
        map!(tuple!(take_until_and_consume!("="), take_until!("\0")),
             |(x, y): (&[u8], &[u8])| {
                 (OsString::from(OsStr::from_bytes(x)), OsString::from(OsStr::from_bytes(y)))
             }));

    terminated!(input,
        separated_list!(tag!("\0"), pair),
        tag!("\0")
    )
}

That brings it down from two to one vector allocated. Unfortunately this still isn't covering the leading = case, though. I think for that it won't be able to use take_until_and_consume! (but maybe there's some nom magic I'm not aware of).

mexus · 2017-08-27T21:29:14Z

src/pid/environ.rs

-        match pair(&self.data) {
-            IResult::Done(leftover, parsed) => Some(Ok((leftover.to_vec(), parsed))),
+/// An iterator over environment variables.
+pub struct EnvironIter<'a> {


I'm in doubt about naming and whether the structure should be exported to a top-level (pid::EnvironIter) or not (I think not).

mexus · 2017-08-27T21:29:46Z

src/pid/environ.rs

+    // Calculate position of the equal sign.
+    let pos = match src.iter().skip(1).position(|c| c == &b'=') {
+        Some(p) => p,
+        None => return IResult::Error(error_position!(nom::ErrorKind::Custom(0), src)),


This is a fishy part, not sure which error to return.

mexus · 2017-08-27T21:30:55Z

src/pid/environ.rs

+    assert!(next.is_some());
+    let next = next.unwrap();
+    assert!(next.is_ok());
+    let next = next.unwrap();


At first I was doing this unwraps w/o is_* checks and it ended up in a mess when an actual error has happened.

mexus · 2017-08-27T21:44:56Z

That brings it down from two to one vector allocated

Pls have a look at yet another version. Sorry for keeping you busy with this.

mexus · 2017-09-01T11:24:48Z

Hi Dan,
did you happen to have a time to have a look at the latest patch?

Also I'd like to make things clear: do you want me to go with allocating a vector that contains all the pairs in one go, or do you like the latest streaming approach? I would rather go with iterators since it's the most idiomatic way as for me. I think I fixed all the extra allocations and manipulations so it should be fine given that I didn't miss anything this time.

mexus · 2017-09-06T18:52:58Z

Since so much time has passed you've probably forgotten how did it start so I squashed everything together.

danburkert

I left comments assuming an Iterator based API. I still think it may be better just to use a vector API, since that way the parsing can be done up front, and a single error returned, instead of potentially returning an error for every pair.

danburkert · 2017-09-09T17:14:56Z

src/pid/environ.rs

+}
+
+/// An iterator over environment variables.
+pub struct EnvironIter<'a> {


Can Environ and EnvironIter be combined into a single type? I think it should be possible just using a Vec<u8> data field and a usize offset.

danburkert · 2017-09-09T17:15:40Z

src/pid/environ.rs

+    type Item = Result<(OsString, OsString)>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        if self.data.len() == 0 {


prefer is_empty.

danburkert · 2017-09-09T17:17:48Z

src/pid/environ.rs

+
+
+#[test]
+fn test_iter() {


Wrap all of the tests in a #[cfg(test)] mod tests { like other files, and put them all at the end of the file.

danburkert · 2017-09-09T17:19:05Z

src/pid/environ.rs

+/// Executes `take_until_and_consume` and maps its result to OsString.
+named_args!(
+    until<'a>(sep: &'a str)<OsString>,
+    map!(take_until_and_consume!(sep), from_bytes)


I think the body here is simple enough just to inline into pair.

mexus · 2018-02-17T12:35:46Z

Sorry, I totally forgot about this pr. I think I will reiterate it once again in a couple of days considering all the review comments.

mexus · 2018-02-17T17:37:45Z

I've kept EnvironIter in order to pass the data to a user as OsStrs instead of allocating OsStrings every time.

I've also put some examples in the test module that show how to easily collect all the name-value pairs into different collections (without having to handle errors for every given pair).

mexus · 2018-09-15T23:33:14Z

Hi @danburkert ! What do you think about my edits? Should I make some further amendments/improvements?

mexus force-pushed the master branch from 62ac86c to ba19a23 Compare August 26, 2017 22:34

danburkert reviewed Aug 26, 2017

View reviewed changes

mexus force-pushed the master branch from ba19a23 to f7ffd4a Compare August 27, 2017 00:06

mexus force-pushed the master branch from f7ffd4a to dfc0b1e Compare August 27, 2017 01:12

danburkert requested changes Aug 27, 2017

View reviewed changes

mexus force-pushed the master branch from dfc0b1e to 1804718 Compare August 27, 2017 11:44

mexus force-pushed the master branch 3 times, most recently from 5e4d63d to 4f09397 Compare August 27, 2017 13:09

mexus commented Aug 27, 2017

View reviewed changes

mexus force-pushed the master branch from 7fc92bb to b679b3f Compare September 6, 2017 18:52

danburkert reviewed Sep 9, 2017

View reviewed changes

mexus force-pushed the master branch from b679b3f to 489cac6 Compare February 17, 2018 17:34

Added parser for /proc/<pid>/environ

e6a73a0

mexus force-pushed the master branch from 489cac6 to e6a73a0 Compare February 17, 2018 17:39



		#[test]
		fn test_iter() {

Added parser for /proc/[pid]/environ #28

Are you sure you want to change the base?

Added parser for /proc/[pid]/environ #28

Conversation

mexus commented Aug 26, 2017

danburkert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mexus commented Aug 26, 2017

mexus commented Aug 27, 2017

danburkert commented Aug 27, 2017

danburkert commented Aug 27, 2017

mexus commented Aug 27, 2017

danburkert commented Aug 27, 2017 via email

danburkert commented Aug 27, 2017 via email

mexus commented Aug 27, 2017

danburkert commented Aug 27, 2017 via email

mexus commented Aug 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mexus commented Aug 27, 2017

mexus commented Aug 27, 2017

danburkert commented Aug 27, 2017

mexus commented Aug 27, 2017 • edited Loading

danburkert commented Aug 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mexus commented Aug 27, 2017

mexus commented Sep 1, 2017

mexus commented Sep 6, 2017

danburkert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mexus commented Feb 17, 2018 • edited Loading

mexus commented Feb 17, 2018 • edited Loading

mexus commented Sep 15, 2018

Added parser for `/proc/[pid]/environ` #28

Added parser for `/proc/[pid]/environ` #28

mexus commented Aug 27, 2017 •

edited

Loading

mexus commented Feb 17, 2018 •

edited

Loading

mexus commented Feb 17, 2018 •

edited

Loading