[wip] API to build BPF programs #6

alex-dukhno · 2017-01-20T21:01:24Z

Hi, I want to learn BPF too 😄

I decided to start from instruction set. This PR was created just to share my idea how API of generating BPF instruction could look like.

I look forward to hearing your opinion about it.

qmonnet

Hey @alex-diez, thanks a lot for this RFC, the API looks really promising! I had in mind to make an assembler in the same way as uBPF does, but I had not really thought about any particular Rust API to build a program so far, and the one you started looks just fine.

I take it you would eventually merge it in the crate, and not as a separate crate as in this PR, is this correct?

I added inline comments about my thoughts on the implementation of the API (keep in mind I am no Rust expert, I may be wrong). Overall I have two issues with this first version:

Some naming choices for structs or functions may be misleading and could be improved in my opinion;
I do not like so much that struct Operations should have a program as an attribute. See details in comments.

Anyway, great work, thanks for working on this!

qmonnet · 2017-01-21T18:07:48Z

instructions/src/lib.rs

+pub enum MemSize {
+    Double,
+    Half
+}


(Missing Word and Byte here, but not a problem since it's work in progress)

I will add other two enum's variants some day with TDD 😉

qmonnet · 2017-01-21T18:24:40Z

instructions/src/lib.rs

+
+    pub fn bind_register(&'p mut self) -> Operation<'p> {
+        Operation::new(self, 0xbf)
+    }


The naming for bind_register() and bind_value() does not really sound familiar for BPF operations. I think it would be easier for users if we have name closer to what is used already in the BPF ecosystem, something like move_value() (or immediate), or move_from_reg(), or anything closer to mov than bind. At least I feel it would help me. Opinions?

I will change to move, it is more domain specific name

qmonnet · 2017-01-21T18:40:05Z

instructions/src/lib.rs

+            MemSize::Half => 0x69
+        };
+        Operation::new(self, opcode)
+    }


Nice to have a single load_mem() function for all memory sizes!

qmonnet · 2017-01-21T18:45:50Z

instructions/src/lib.rs

+        }
+        self.program
+    }
+}


So most of the functions in struct Operation could probably be moved to ebpf::Insn, or to another struct Instruction that has a ebpf::Insn attribute maybe, or something like that. Not the push() function, though. I think it would make sense to move it to the ProgramCodeBuilder? Would that work? What's your opinion about this?

I am not sure about that yet. I wanted to write a BPF program by calling chain of functions.
For instance:

program.bind_value().dst(0x00).value(0x00_00_11_22).push().bind_value().dst(0x01).value(0x11_22_00_00).push().add_registers().dst(0x00).src(0x01).push() ... /* and so forth */.exit()

Here I called function push referring push on stack, however I am not sure that it is a good name for it.

However, I have not considered to write it in this way:

program.add(Instruction::new(Class::Move).dst(0x00).src(0x01));

qmonnet · 2017-01-21T18:47:11Z

instructions/src/lib.rs

+    Half
+}
+
+pub struct ProgramCodeBuilder {


Or just ProgramBuilder, or ProgBuilder? The Code makes it longer to type/read, and I am not sure it brings additional information?

qmonnet · 2017-01-21T18:50:42Z

instructions/src/lib.rs

+            ins.push((self.immediate & 0x00_00_00_FF) as u8);
+            ins.push(((self.immediate & 0x00_00_FF_00) >> 8) as u8);
+            ins.push(((self.immediate & 0x00_FF_00_00) >> 16) as u8);
+            ins.push(((self.immediate & 0xFF_00_00_00) >> 24) as u8);


Nitpicking for style consistency: I used lowercase letters for other hexadecimal values in the crate. Anyway, do keep the underscores, I did not know we could use them—thanks!—and I'll add them in the rest of the crate to make it more readable.

qmonnet · 2017-01-21T18:52:05Z

instructions/src/lib.rs

+}
+
+#[cfg(test)]
+mod tests {


Nice to have unit tests, thanks for that!

qmonnet · 2017-01-21T18:53:44Z

instructions/src/lib.rs

+        self
+    }
+
+    pub fn value(&'p mut self, immediate: u32) -> &'p mut Operation<'p> {


immediate() or imm() instead of value()?

alex-dukhno · 2017-01-21T20:58:58Z

I take it you would eventually merge it in the crate, and not as a separate crate as in this PR, is this correct?

Yes, it is just faster to run cargo test on separated crate 😄

(keep in mind I am no Rust expert, I may be wrong)

Me either, I just write tests for each functionality that I want to implement and run cargo test as frequently as possible to make sure that compiler is not arguing and everything works as I want 😉

I read through you comments ... Anyway I am developing in TDD so names and missed MemSize's variants will be fixed eventually. Actually, by this PR I wanted to show you chain API and hear your opinion about it. I have rewritten one of you test to show how it looks like in real world. That's it.

Long names don't bother me a lot for now. Auto-complition does its job well.

qmonnet · 2017-01-21T21:53:39Z

Yeah, the new version of the test is way more readable than the slice of hexa code… I'd be glad to merge your API once it's ready.

Long names don't bother me a lot for now. Auto-complition does its job well.

Sure, but then there's no need to clutter the doc with long names if you gain nothing by making them long ;-). In the case of ProgramCodeBuild, that was just a suggestion, it's not that long and I'm fine with it if you really want to keep it.

alex-dukhno · 2017-01-29T15:25:52Z

Writing tests for load instructions I noticed something interesting.
Instructions

Op code	Mnemonic
0x18	lddw dst, imm
0x20	ldabsw src, dst, imm
0x28	ldabsh src, dst, imm
0x30	ldabsb src, dst, imm
0x38	ldabsdw src, dst, imm
0x40	ldindw src, dst, imm
0x48	ldindh src, dst, imm
0x50	ldindb src, dst, imm
0x58	ldinddw src, dst, imm

according LD/LDX/ST/STX opcode structure

msb      lsb
+---+--+---+
|mde|sz|cls|
+---+--+---+

have 000 class, but other loads have 001 class.

Instructions in the above table, are they some kind of special? Does their class have special name in terms of BPF?

Edit: I got it. 000 means that address is immediate and 001 means that address is in src register

qmonnet · 2017-01-29T21:37:21Z

That's it, the difference is the source of the load operand, immediate or register. As you surely noticed already, it uses these definitions from src/ebpf.rs:

pub const BPF_LD    : u8 = 0x00;
pub const BPF_LDX   : u8 = 0x01;

(If I remember correctly the previous BPF version had a register called X for this kind of operations, that's probably where we get the name from.)

alex-dukhno · 2017-01-31T19:08:33Z

Pushed second version of Rust API.

I have a couple of questions:

should I rewrite unit/integration tests with my API, if it is Ok?
should I merge ebpf::Insn structure and related functions with my API, if it is Ok?

alex-dukhno · 2017-01-31T19:11:01Z

instructions/src/lib.rs

+}
+
+impl Load {
+    pub fn new(mem_size: MemSize, addressing: Addressing, address_source: AddressSource) -> Self {


I am not sure about this Addressing and AddressSource enums, though. 😕

Probably easier to decouple the different kinds of load into several operation type instead of using this Adressing everywhere? Maybe also for the AddressSource I prefer what you had in your v1?

qmonnet · 2017-02-01T08:35:35Z

Hey thanks for the hard work, that's impressive! Please can you give me a few days so that I can study and play with it? I don't have too much time right now to dive into the details.

Please do not start any rewriting of the tests coming from uBPF (tests/upbf_*) for now. We should also have an assembler soon, and may want to build these programs with this assembler so as to better follow the evolution of uBPF test suite. Once merged, your API could be used for all other unit tests—that's not so many tests for now, but I wish to extend the list, we have many uncovered error cases.

qmonnet · 2017-02-06T20:13:25Z

OK so, sorry for the delay, here's some feedback.

I don't really know how to say it with tact, but the thing is, I preferred your first version with chaining the commands. I know I told you having a program as an attribute in the Operation seemed strange to me, but on a second thought, I just believe I told you crap. I first thought that we would have to add this program reference to the ebpf::Insn struct to merge it all, but I realize your Operation is something different. I can't find a name that would ideally describe it, maybe an InsnBuilder to match the ProgramBuilder. Well anyway, I think it could have an ebpf::Insn attribute in the end, instead of having again all insn fields, and it would look great.

Regarding v2, I tend to find it quite verbose, and too “low-level”. I realize you've made a great effort to reconstruct the operation codes with the different class / memory / etc. codes, but I don't believe the user is interested in this level of details, and should have so many steps to build an instruction (for example, using Move to perform an addition or multiplication or other ALUs is not exactly intuitive). Plus we will have an assembler to produce bytecode from uBPF-like assembly, which is higher level than the v2 of the API… In contrast, the .load_mem() function from v1 were easier to use I think.

The idea to have a different struct for each operation type is good, though. Maybe we could push this further and restrict the operand-related methods to fill only the operands that are relevant for the current operation, raising an error/panic!ing otherwise?

So again, sorry for changing my mind, I realize you've done a lot on this v2. What's your opinion on the matter, do you think one version is specifically better than the other?

qmonnet · 2017-02-06T20:19:35Z

instructions/src/lib.rs

+}
+
+impl Load {
+    pub fn new(mem_size: MemSize, addressing: Addressing, address_source: AddressSource) -> Self {


Probably easier to decouple the different kinds of load into several operation type instead of using this Adressing everywhere? Maybe also for the AddressSource I prefer what you had in your v1?

qmonnet · 2017-02-06T20:21:37Z

instructions/src/lib.rs

+            Addressing::Ind => 0x40
+        }
+    }
+}


Not sure we have to use a match for extracting values from enums. Apparently you can give each item a discriminant value, and then cast as an integer type, see https://doc.rust-lang.org/reference.html#enumerations.

Thanks. Didn't know that.

qmonnet · 2017-02-06T20:23:29Z

instructions/src/lib.rs

+    src: u8,
+    offset: u16,
+    imm: u32
+}


imm and offset are considered as signed. We'll have to fix it at some point if we use ebpf::Insn.

alex-dukhno · 2017-02-07T18:31:36Z

Thanks for the review.

First of all v2 was just a try 😄 as v1. The more Rust code I write the better Rust developer I become. That's one of a reason why I start contributing to this project 😉

When I finished v2 I thougth that I would have to merge it with ebpf::Insn somehow. However, after #9 PR I think we need to develop Instruction API which will be the same for ebpf, disassembler and asm_parser modules and Rust BPF Builder API (this PR). What do you think about that?

qmonnet · 2017-02-11T19:44:54Z

Hi @alex-diez, and sorry for being so slow to answer, I'm on a business trip and the WiFi is awful…

It might be nice to unify the instructions for API/assembler/disassembler, I haven't given a thought into that so far. But I'm not sure I see what you have in mind with this Instruction API, what would it look like and what would it bring exactly?

I'm being very cautious with the instructions in ebpf module, though. As I see it, this module should remain self-contained and only define stuff that is generic for eBPF. As such, it could ultimately be reused in other projects that need eBPF stuff but have nothing to do with userspace VM or (dis)assemblers. I don't know if this can have some impact on what you had in mind, I prefer to warn just in case :)

* add Assemble trait * add Disassemble trait

alex-dukhno · 2017-02-19T11:17:14Z

Pushed last changes.

I left some todos.

I think BpfCode is better to call RawBpfCode.
Add static functions (I am not sure about arguments types):
- from_elf(path_elf: &str)
- parse(asm: &str)
Create VerifiedBpfCode struct, which will be created by calling fn verify() -> Result<VerifiedBpfCode, VerificationError>.

For example:

let bpf_code = RawBpfCode::from_elf("/path/to/elf_file");
let verified_code = bpf_code.verify().unwrap();

let vm = rbpf::EbpfVmNoData::new(verified_code.assemble());
vm.prog_exec();

qmonnet

Thanks @alex-diez!

I like the current shape of the API used to build the programs, but I still have several comments.

The PR is getting hard to review, since commits are stacked on one another. I forked on my side and squashed your commit to have a clean view of the changes. That's a detail. But also, you push a lot of changes in a single PR;
In particular, I am not convinced having this Dissemble trait and implementation in this API is a good thing (and not in the same PR at any rate). We already have a disassembler, so you'll have to convince me we need another here :). Plus I already feel concerned that the assembler and disassembler we have do not share the same text strings and we'll have to keep them in sync if any of them moves, so I'm not very keen on adding a third similar component. Thoughts?
By the way, Assemble / Asm names are really close to what we have in the assembler.rs module, may lead to confusion. Maybe Build? (Or more figurative, Shape, Craft? I don't know.)
Same remark for parse, from_elf and verify: please do not implement them in this PR. I am not sure we really need any of this, so it might be worth talking about it first before implementing?

This being said, I do like the current basic API to build the programs (without Disassemble trait I mean), you could consider moving this code to its own module so that you can access ebpf::Insn and finalize a version we could merge.

qmonnet · 2017-02-19T12:39:37Z

instructions/src/lib.rs

 }

 #[cfg(test)]
 mod tests {
    #[cfg(test)]
+    mod disassembling {
+        use super::super::*;
+


Trailing whitespaces 1/3

qmonnet · 2017-02-19T12:40:13Z

instructions/src/lib.rs

+        #[test]
+        fn exit() {
+            let mut program = BpfCode::new();
+


Trailing whitespaces 2/3

qmonnet · 2017-02-19T12:40:16Z

instructions/src/lib.rs

+
+            assert_eq!(call.disassemble(), "call");
+        }
+


Trailing whitespaces 3/3

qmonnet · 2017-02-19T13:14:51Z

instructions/src/lib.rs

    pub fn new() -> Self {
        BpfCode { instructions: vec![] }
    }

-    pub fn as_bytes(&self) -> &[u8] {
-        self.instructions.as_slice()
+    pub fn mov_add(&mut self, source: Source, arch: Arch) -> Move {


The mov_ prefix, here and in the following functions, is misleading to new eBPF users. I understand you refer to the operation class that is used to form the operation code, but I don't think the user should have to care about that. Thoughts?

alex-dukhno · 2017-02-19T14:40:38Z

First of all, I don't want to have two disassemble APIs. 😄

I can remove Disassemble trait and merge into a separate module.

PS: working on this API I start thinking about this RawBpfCode and VerifiedBpfCode and let myself create needless functionality. And I understood the reason. It is because my code is OOP-like rather then procedural-like as in the other modules. 😄
I start to imagine that we could write chaining code for BPF program in general. For instance:

let bpf_vm = ... // create VM of any type

RawBpfCode::new().load(...).push().store(...).push()
// or RawBpfCode::parse(...)
// or RawBpfCode::from_elf(...)
.verify().map(|verified| verified.execute_with(bpf_vm)).err().map(...);

and RawBpfCode::parse() can actually use asm_parser functionality to be synched with it.

PS2: Maybe it is better to create an Issue and move this discussion there or should I just provide PR without Disassemble trait with current API?

qmonnet · 2017-02-19T17:35:51Z

I can remove Disassemble trait and merge into a separate module.

Sure, thanks!

I start to imagine that we could write chaining code for BPF program in general.

Well, we most probably could. But is this something we should do? This is a more OOP approach indeed, but does it make the lib easier to use? I mean, we already have the “procedural-like” approach that works well, why change it (real question, I'm open to listening to arguments)? Opening an issue about may be a good idea indeed. I am not sure anyone else will immediately take part in the discussion, but this is something we can leave open a while and see if other people express interest in it.

qmonnet

Thanks once more @alex-diez! This is getting better. I still have (not-so-)minor comments. In particular, it's great to have those per-instruction-class structs (they add some constraints to keep users from setting wrong fields), but this makes the code lengthy and we could probably refactor it. I believe we can put a default definition of the functions for the trait in the trait declaration, might be worth to try?

qmonnet · 2017-02-21T20:59:06Z

src/rust_api.rs

@@ -0,0 +1,1561 @@
+// Copyright 2016 6WIND S.A. <quentin.monnet@6wind.com>


Thanks for the boilerplate, but I'm not the author of this file! ;) Just set the copyright to your own name (or company if need be).

qmonnet · 2017-02-21T20:59:08Z

src/lib.rs

@@ -28,6 +28,7 @@ pub mod ebpf;
 pub mod helpers;
 mod jit;
 mod verifier;
+pub mod rust_api;


We can probably find a better name. Of course it's Rust, all the project is ;). Not sure also we need to specify it is an API. The VM and JIT offer an API also. Maybe something like builder or prog_build or whatever?

Once you have picked a name, could you please:

insert the line in the list of public module inclusions, in the alphabetical order,

and make sure this compiles on top of the master branch?

code_build, insn_build 🤔

Hmm I don't know. I don't like code_build much, we're not building “code”. I would accept insn_build I think. We can keep searching for a name until we merge. Should ideally be short, for people that do not include <module>::* but will prefer to call <module>::<function>() a lot.

qmonnet · 2017-02-21T21:00:15Z

src/rust_api.rs

+        BpfCode { instructions: vec![] }
+    }
+
+    pub fn mov_add(&mut self, source: Source, arch: Arch) -> Move {


Again, I'm not sure the mov_ prefix will help end users understanding what the function does.

qmonnet · 2017-02-21T21:03:28Z

src/rust_api.rs

+        SwapBytes {
+            bpf_code: self,
+            endian: endian,
+            insn: Insn {


I guess I should implement or derive the Default trait for ebpf::Insn…

qmonnet · 2017-02-21T21:07:05Z

src/rust_api.rs

+
+#[derive(Copy, Clone)]
+enum OpBits {
+    Add = 0x00,


Please use the values from ebpf module instead: ebpf::BPF_ADD and so on.

qmonnet · 2017-02-21T21:13:28Z

src/rust_api.rs

+
+#[derive(Copy, Clone, PartialEq)]
+pub enum Cond {
+    Abs = 0x00,


qmonnet · 2017-02-21T21:14:30Z

src/rust_api.rs

+    fn immediate(self, imm: i32) -> Self;
+}
+
+pub trait IntoBytes {


Yay, this name is way better than assemble here!

qmonnet · 2017-02-21T21:16:13Z

src/rust_api.rs

+    }
+
+    #[inline]
+    fn mov_internal(&mut self, source: Source, arch_bits: Arch, op_bits: OpBits) -> Move {


(No objection to keep mov_ for this function, though)

qmonnet · 2017-02-21T21:29:23Z

tests/misc.rs

@@ -253,6 +253,8 @@ fn test_jit_block_port() {
    }
 }

+
+


You somehow left two additional empty lines here, please remove.

qmonnet · 2017-02-21T21:33:46Z

src/rust_api.rs

+
+    /// sets immediate value
+    fn immediate(self, imm: i32) -> Self;
+}


Also the names for the getters and setters are quite similar, and I find them confusing (dst to read the value, dst_reg to set it? And offset_bytes seems uselessly long). Could you find something else? Maybe adding set_ prefix to the setters?

alex-dukhno · 2017-02-22T20:27:20Z

src/rust_api.rs

+    }
+
+    pub fn load_x(&mut self, mem_size: MemSize) -> Load {
+        self.load_internal(mem_size, Addressing::Undef, 0x61)


BPF_MEM = 0x60 ... but I can find the meaning of 0x01 ... only BPF_LDX 🤔

Yes, that's BPF_LDX here I guess. In ebpf module you have for example

pub const LD_B_REG : u8 = BPF_LDX | BPF_MEM | BPF_B;

(for “load byte from register” operation), or from Linux (linux/kernel/bpf/core.c):

[BPF_LDX | BPF_MEM | BPF_B] = &&LDX_MEM_B,

I had better make last changes on weekends ... Yesterday evening I saw store_x instead of load_x. Hallucinations 😄

alex-dukhno · 2017-02-23T17:36:54Z

src/rust_api.rs

+    }
+
+    pub fn store_x(&mut self, mem_size: MemSize) -> Store {
+        self.store_internal(mem_size, 0x61)


Nope, I didn't have hallucinations yesterday 😄. Here, I didn't know what constant I should use. BPF_MEM | BPF_STX works.

Well, maybe you simply should not have 0x61 there? I think it should be BPF_STX | BPF_MEM, so 0x63 maybe? I haven't checked whether the tests pass if we change this value.

alex-dukhno · 2017-02-25T19:44:23Z

Forget to add changed files

qmonnet

It seems you manage to implement the functions in the trait after all, that's great!

We're nearly good, I only suggest a small change for the code this time. But then we have all the warnings because of the missing documentation, could you add it for all the items that are publicly exported? I know this is not the funniest part, but that's quite mandatory for an API if we want people to use it.

qmonnet · 2017-02-26T00:49:45Z

src/insn_builder.rs

+        buffer.push(((self.get_imm() & 0xff_00_00_00) >> 24) as u8);
+        buffer
+    }
+}


Two warnings are raised by this function, regarding the int values that are used for the masks on lines 79 and 83. In fact, all those masks are useless, and you can do as follows:

fn into_bytes(self) -> Self::Bytes { let mut buffer = Vec::with_capacity(8); buffer.push( self.opt_code_byte()); buffer.push( self.get_src() << 4 | self.get_dst()); buffer.push( self.get_off() as u8); buffer.push((self.get_off() >> 8) as u8); buffer.push( self.get_imm() as u8); buffer.push((self.get_imm() >> 8) as u8); buffer.push((self.get_imm() >> 16) as u8); buffer.push((self.get_imm() >> 24) as u8); buffer }

Casting to u8 after the division automatically trims the unwanted part on the “left”.

(Of course you don't have to keep the same formating, aligning stuff is just my personal preference in such case).

qmonnet · 2017-02-26T00:51:15Z

src/insn_builder.rs

@@ -0,0 +1,1389 @@
+// Copyright 2017 <alex.dukhno@icloud.com>


Maybe with your name in addition to the email?

// Copyright 2017 Alex Dukhno <alex.dukhno@icloud.com>

qmonnet · 2017-02-26T13:03:30Z

Ok, warnings are gone, I think we're good this time! :). The commit history on this PR is a bit chaotic though, so I intend to squash commits. Is is ok for you, or do you prefer to split your changes yourself into a new set of (logical) commits?

alex-dukhno · 2017-02-26T13:25:25Z

Just squash it

qmonnet · 2017-02-26T13:56:02Z

Done!

qmonnet · 2017-02-26T13:56:55Z

100th commit, by the way

alex-dukhno · 2017-02-26T14:02:12Z

Anniversary 😄

[wip] API to build BPF programs

78a8ce8

qmonnet reviewed Jan 21, 2017

View reviewed changes

[wip] API to build BPF programs (version 2)

dadefa7

alex-dukhno commented Jan 31, 2017

View reviewed changes

qmonnet reviewed Feb 6, 2017

View reviewed changes

qmonnet mentioned this pull request Feb 15, 2017

eBPF assembler #10

Merged

third version of Rust API

edcee28

* add Assemble trait * add Disassemble trait

qmonnet reviewed Feb 19, 2017

View reviewed changes

Alex-Diez and others added 4 commits February 21, 2017 20:07

move from crate to module

1d0844d

add test_vm_mbuff with rust_api

5cbc93d

Merge branch 'master' into operations

10c2de7

remove second disassemble mod declaration

cdf56fc

alex-dukhno mentioned this pull request Feb 21, 2017

Chained API for assemble / verify / run BPF programs #12

Open

qmonnet requested changes Feb 21, 2017

View reviewed changes

alex-dukhno commented Feb 22, 2017

View reviewed changes

alex-dukhno commented Feb 23, 2017

View reviewed changes

Alex-Diez added 2 commits February 23, 2017 19:54

replace magic numbers with constants

ef9af6d

reduce boilerplate code

fd4a52c

rename module to insn_builder

bf0ea54

add missed files

0067ba7

qmonnet reviewed Feb 26, 2017

View reviewed changes

add documentaion

9d67bae

qmonnet merged commit 724fee5 into qmonnet:master Feb 26, 2017

alex-dukhno mentioned this pull request Mar 5, 2017

Instruction stack data structure #14

Open

		@@ -0,0 +1,1561 @@
		// Copyright 2016 6WIND S.A. <quentin.monnet@6wind.com>

		@@ -0,0 +1,1389 @@
		// Copyright 2017 <alex.dukhno@icloud.com>

[wip] API to build BPF programs #6

[wip] API to build BPF programs #6

Conversation

alex-dukhno commented Jan 20, 2017

qmonnet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-dukhno Jan 21, 2017 • edited Loading

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

Choose a reason for hiding this comment

alex-dukhno commented Jan 21, 2017 • edited Loading

qmonnet commented Jan 21, 2017

alex-dukhno commented Jan 29, 2017 • edited Loading

qmonnet commented Jan 29, 2017

alex-dukhno commented Jan 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qmonnet commented Feb 1, 2017

qmonnet commented Feb 6, 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

alex-dukhno commented Feb 7, 2017

qmonnet commented Feb 11, 2017

alex-dukhno commented Feb 19, 2017

qmonnet 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

alex-dukhno commented Feb 19, 2017

qmonnet commented Feb 19, 2017

qmonnet 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

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

Choose a reason for hiding this comment

alex-dukhno Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-dukhno commented Feb 25, 2017 • edited Loading

qmonnet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qmonnet commented Feb 26, 2017

alex-dukhno commented Feb 26, 2017

qmonnet commented Feb 26, 2017

qmonnet commented Feb 26, 2017

alex-dukhno commented Feb 26, 2017

alex-dukhno Jan 21, 2017 •

edited

Loading

alex-dukhno commented Jan 21, 2017 •

edited

Loading

alex-dukhno commented Jan 29, 2017 •

edited

Loading

alex-dukhno Feb 23, 2017 •

edited

Loading

alex-dukhno commented Feb 25, 2017 •

edited

Loading