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

[mexus]

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.

[mexus]

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

[mexus]

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

[danburkert]

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]

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]

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]

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 notifications@github.com 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 https://github.com/danburkert/procinfo-rs/pull/28#issuecomment-325170204, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJx8miZMi9cA7KZawmTnH2J9KnOsnKTks5scL3FgaJpZM4PDooe .

[danburkert]

Can't *

On Sat, Aug 26, 2017 at 6:03 PM Dan Burkert dan@danburkert.com 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 notifications@github.com 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 https://github.com/danburkert/procinfo-rs/pull/28#issuecomment-325170204, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJx8miZMi9cA7KZawmTnH2J9KnOsnKTks5scL3FgaJpZM4PDooe .

[mexus]

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]

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

On Sat, Aug 26, 2017 at 6:03 PM Dan Burkert dan@danburkert.com wrote:

Can't *

On Sat, Aug 26, 2017 at 6:03 PM Dan Burkert dan@danburkert.com 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 notifications@github.com 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 https://github.com/danburkert/procinfo-rs/pull/28#issuecomment-325170204, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJx8miZMi9cA7KZawmTnH2J9KnOsnKTks5scL3FgaJpZM4PDooe .

[mexus]

Anything else? :)

[mexus]

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

[mexus]

Oh man that old rust compiler makes me sick

[danburkert]

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]

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]

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]

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]

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]

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

[mexus]

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]

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]

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